-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[OMPIRBuilder] always leave PARALLEL via the same barrier #164586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1108,21 +1108,9 @@ OpenMPIRBuilder::createCancel(const LocationDescription &Loc, | |
| Value *Args[] = {Ident, getOrCreateThreadID(Ident), CancelKind}; | ||
| Value *Result = Builder.CreateCall( | ||
| getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_cancel), Args); | ||
| auto ExitCB = [this, CanceledDirective, Loc](InsertPointTy IP) -> Error { | ||
| if (CanceledDirective == OMPD_parallel) { | ||
| IRBuilder<>::InsertPointGuard IPG(Builder); | ||
| Builder.restoreIP(IP); | ||
| return createBarrier(LocationDescription(Builder.saveIP(), Loc.DL), | ||
| omp::Directive::OMPD_unknown, | ||
| /* ForceSimpleCall */ false, | ||
| /* CheckCancelFlag */ false) | ||
| .takeError(); | ||
| } | ||
| return Error::success(); | ||
| }; | ||
|
|
||
| // The actual cancel logic is shared with others, e.g., cancel_barriers. | ||
| if (Error Err = emitCancelationCheckImpl(Result, CanceledDirective, ExitCB)) | ||
| if (Error Err = emitCancelationCheckImpl(Result, CanceledDirective)) | ||
| return Err; | ||
|
|
||
| // Update the insertion point and remove the terminator we introduced. | ||
|
|
@@ -1159,21 +1147,9 @@ OpenMPIRBuilder::createCancellationPoint(const LocationDescription &Loc, | |
| Value *Args[] = {Ident, getOrCreateThreadID(Ident), CancelKind}; | ||
| Value *Result = Builder.CreateCall( | ||
| getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_cancellationpoint), Args); | ||
| auto ExitCB = [this, CanceledDirective, Loc](InsertPointTy IP) -> Error { | ||
| if (CanceledDirective == OMPD_parallel) { | ||
| IRBuilder<>::InsertPointGuard IPG(Builder); | ||
| Builder.restoreIP(IP); | ||
| return createBarrier(LocationDescription(Builder.saveIP(), Loc.DL), | ||
| omp::Directive::OMPD_unknown, | ||
| /* ForceSimpleCall */ false, | ||
| /* CheckCancelFlag */ false) | ||
| .takeError(); | ||
| } | ||
| return Error::success(); | ||
| }; | ||
|
|
||
| // The actual cancel logic is shared with others, e.g., cancel_barriers. | ||
| if (Error Err = emitCancelationCheckImpl(Result, CanceledDirective, ExitCB)) | ||
| if (Error Err = emitCancelationCheckImpl(Result, CanceledDirective)) | ||
| return Err; | ||
|
|
||
| // Update the insertion point and remove the terminator we introduced. | ||
|
|
@@ -1277,8 +1253,7 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::emitKernelLaunch( | |
| } | ||
|
|
||
| Error OpenMPIRBuilder::emitCancelationCheckImpl( | ||
| Value *CancelFlag, omp::Directive CanceledDirective, | ||
| FinalizeCallbackTy ExitCB) { | ||
| Value *CancelFlag, omp::Directive CanceledDirective) { | ||
| assert(isLastFinalizationInfoCancellable(CanceledDirective) && | ||
| "Unexpected cancellation!"); | ||
|
|
||
|
|
@@ -1305,13 +1280,17 @@ Error OpenMPIRBuilder::emitCancelationCheckImpl( | |
|
|
||
| // From the cancellation block we finalize all variables and go to the | ||
| // post finalization block that is known to the FiniCB callback. | ||
| Builder.SetInsertPoint(CancellationBlock); | ||
| if (ExitCB) | ||
| if (Error Err = ExitCB(Builder.saveIP())) | ||
| return Err; | ||
| auto &FI = FinalizationStack.back(); | ||
| if (Error Err = FI.FiniCB(Builder.saveIP())) | ||
| return Err; | ||
| if (!FI.FiniBB) { | ||
| llvm::IRBuilderBase::InsertPointGuard Guard(Builder); | ||
| FI.FiniBB = BasicBlock::Create(BB->getContext(), ".fini", BB->getParent()); | ||
| Builder.SetInsertPoint(FI.FiniBB); | ||
| // FiniCB adds the branch to the exit stub. | ||
| if (Error Err = FI.FiniCB(Builder.saveIP())) | ||
| return Err; | ||
| } | ||
| Builder.SetInsertPoint(CancellationBlock); | ||
| Builder.CreateBr(FI.FiniBB); | ||
|
|
||
| // The continuation block is where code generation continues. | ||
| Builder.SetInsertPoint(NonCancellationBlock, NonCancellationBlock->begin()); | ||
|
|
@@ -1800,8 +1779,18 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::createParallel( | |
| Instruction *PRegPreFiniTI = PRegPreFiniBB->getTerminator(); | ||
|
|
||
| InsertPointTy PreFiniIP(PRegPreFiniBB, PRegPreFiniTI->getIterator()); | ||
| if (Error Err = FiniCB(PreFiniIP)) | ||
| return Err; | ||
| if (!FiniInfo.FiniBB) { | ||
| if (Error Err = FiniCB(PreFiniIP)) | ||
| return Err; | ||
| } else { | ||
| IRBuilderBase::InsertPointGuard Guard{Builder}; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. brace-initialization is uncommon in |
||
| Builder.restoreIP(PreFiniIP); | ||
| Builder.CreateBr(FiniInfo.FiniBB); | ||
| // There's currently a branch to omp.par.exit. Delete it. We will get there | ||
| // via the fini block | ||
| if (llvm::Instruction *Term = Builder.GetInsertBlock()->getTerminator()) | ||
| Term->eraseFromParent(); | ||
| } | ||
|
|
||
| // Register the outlined info. | ||
| addOutlineInfo(std::move(OI)); | ||
|
|
@@ -6556,13 +6545,14 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::emitCommonDirectiveExit( | |
| FinalizationInfo Fi = FinalizationStack.pop_back_val(); | ||
| assert(Fi.DK == OMPD && "Unexpected Directive for Finalization call!"); | ||
|
|
||
| if (Error Err = Fi.FiniCB(FinIP)) | ||
| return Err; | ||
|
|
||
| BasicBlock *FiniBB = FinIP.getBlock(); | ||
| Instruction *FiniBBTI = FiniBB->getTerminator(); | ||
| if (!Fi.FiniBB) { | ||
| if (Error Err = Fi.FiniCB(FinIP)) | ||
| return Err; | ||
| Fi.FiniBB = FinIP.getBlock(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [serious] Is there a guarantee that (Note: this is why CanonicalLoopInfo has a predefined control-flow structure) |
||
| } | ||
|
|
||
| // set Builder IP for call creation | ||
| Instruction *FiniBBTI = Fi.FiniBB->getTerminator(); | ||
| Builder.SetInsertPoint(FiniBBTI); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would set
FiniInfo.FiniBB?If it is a call to
createBarrierthat theFiniCBis expected to make, that's the kind of devil's contract that makes the callback-driven design so bad.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The finalisation basic block could have already been created if the body of the parallel operation contained a cancellation point or cancel. In that case we should just branch straight to the block created previously. I agree the control flow with all of the callbacks and the cancellation stack are a bit hard to follow. This is not new with this patch.
In most cases, no cancellation will have already created a finalisation block so finalisation should be generated right here as was done before this patch.
The intention here is to only run the finalisation callback once and have all exists branch to that one instance (and also to include the barrier in that unique exit so that all threads block on the same barrier).
It is not expected that the FiniCB creates FiniBB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we improve the situation a bit to make it more predictable? There seem to be two code locations where FiniCB is called and FiniBB is assigned, here or in
commonDirectiveExit. I propose refactoring this out into a different function whose task it is to get the FiniBB: If it was already created, return it; otherwise emit one. Such as