[OMPIRBuilder] Improve 'target if' implementation #222
Conversation
skatrak
requested review from
ergawy,
jsjodin,
mjklemm,
agozillon,
DominikAdamski,
TIFitis and
bhandarkar-pranav
| llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP = [&]() { | ||
| // Assume no error was returned because EmitTargetCallFallbackCB doesn't | ||
| // produce any. | ||
| llvm::OpenMPIRBuilder::InsertPointTy AfterIP = cantFail([&]() { |
There was a problem hiding this comment.
Does not adding cantFail in this case introduce assumptions about the internals of all called functions/callbacks (e.g. in this instance, the internals of emitKernelLaunch)?
The same question also applies to some of the later uses of cantFail in the PR.
There was a problem hiding this comment.
A couple have been added, but most of these assumptions were already there before the change. I just wasn't familiar with the existence of cantFail() at the time, so wherever possible I just forwarded an error knowing it would never happen and in other places I added a comment saying it couldn't fail, but was still checking the result so that it could be used (it causes runtime failures if you don't check these, even if they succeed). llvm#115863 intends to straighten out other places where this is the case.
The idea with introducing Errors to the OMPIRBuilder was that they could only be caused by callbacks passed from outside. That way, if something failed there, the OMPIRBuilder could stop early and forward the error. So, if we have control over all callback contents, as it's the case here, we know in advance that no errors can be returned. In that case, I believe it's better to not forward these errors to the caller because it's misleading about possibly failing and forces callers to handle that impossible error.
This patch cleans up support for the 'if' clause on 'target' directives by reusing the existing `emitIfClause()` function rather than duplicating code. One side effect of this change is that constant 'if' values will no longer result in the generation of branches. Instead, code is generated only for the applicable case. It also adds a unit test to ensure it works as expected.
This patch cleans up support for the 'if' clause on 'target' directives by reusing the existing
emitIfClause()function rather than duplicating code. One side effect of this change is that constant 'if' values will no longer result in the generation of branches. Instead, code is generated only for the applicable case.It also adds a unit test to ensure it works as expected.