-
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
Open
tblah
wants to merge
2
commits into
main
Choose a base branch
from
users/tblah/cancel-bug-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+124
−117
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 { | ||
| llvm::IRBuilderBase::InsertPointGuard Guard{Builder}; | ||
tblah marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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(); | ||
|
Member
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); | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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