-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[flang][OpenMP] Allow saving first block of an OMP region for allocas #121886
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
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 |
|---|---|---|
|
|
@@ -345,31 +345,37 @@ findAllocaInsertPoint(llvm::IRBuilderBase &builder, | |
| allocaInsertPoint = frame.allocaInsertPoint; | ||
| return WalkResult::interrupt(); | ||
| }); | ||
| if (walkResult.wasInterrupted()) | ||
| return allocaInsertPoint; | ||
|
|
||
| // Otherwise, insert to the entry block of the surrounding function. | ||
| // If the current IRBuilder InsertPoint is the function's entry, it cannot | ||
| // also be used for alloca insertion which would result in insertion order | ||
| // confusion. Create a new BasicBlock for the Builder and use the entry block | ||
| // for the allocs. | ||
| if (!walkResult.wasInterrupted()) { | ||
| llvm::BasicBlock &funcEntryBlock = | ||
| builder.GetInsertBlock()->getParent()->getEntryBlock(); | ||
| allocaInsertPoint = llvm::OpenMPIRBuilder::InsertPointTy( | ||
| &funcEntryBlock, funcEntryBlock.getFirstInsertionPt()); | ||
| } | ||
|
|
||
| // If the current IRBuilder insertion block is the same as the alloca | ||
| // insertion block, it cannot also be used for alloca insertion which would | ||
| // result in insertion order confusion. Create a new BasicBlock for the | ||
| // Builder and use the entry block for the allocs. | ||
|
Comment on lines
+357
to
+360
Contributor
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. Shouldn't it be okay so long as the allocas land at the start of the block (as they should be anyway)? |
||
| // | ||
| // TODO: Create a dedicated alloca BasicBlock at function creation such that | ||
| // we do not need to move the current InertPoint here. | ||
| if (builder.GetInsertBlock() == | ||
| &builder.GetInsertBlock()->getParent()->getEntryBlock()) { | ||
| if (builder.GetInsertBlock() == allocaInsertPoint.getBlock()) { | ||
| assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() && | ||
| "Assuming end of basic block"); | ||
| llvm::BasicBlock *entryBB = llvm::BasicBlock::Create( | ||
| builder.getContext(), "entry", builder.GetInsertBlock()->getParent(), | ||
| builder.GetInsertBlock()->getNextNode()); | ||
| builder.CreateBr(entryBB); | ||
| builder.SetInsertPoint(entryBB); | ||
| auto *insertCont = splitBB( | ||
| llvm::OpenMPIRBuilder::InsertPointTy( | ||
| allocaInsertPoint.getBlock(), allocaInsertPoint.getBlock()->end()), | ||
| true, "insert.cont"); | ||
| builder.SetInsertPoint(insertCont, insertCont->end()); | ||
| } | ||
|
|
||
| llvm::BasicBlock &funcEntryBlock = | ||
| builder.GetInsertBlock()->getParent()->getEntryBlock(); | ||
| return llvm::OpenMPIRBuilder::InsertPointTy( | ||
| &funcEntryBlock, funcEntryBlock.getFirstInsertionPt()); | ||
| allocaInsertPoint.getBlock(), | ||
| allocaInsertPoint.getPoint() != allocaInsertPoint.getBlock()->end() | ||
| ? allocaInsertPoint.getPoint() | ||
| : allocaInsertPoint.getBlock()->getFirstInsertionPt()); | ||
|
Comment on lines
+376
to
+378
Contributor
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. Why not unconditionally put the alloca at the start of the block? |
||
| } | ||
|
|
||
| /// Converts the given region that appears within an OpenMP dialect operation to | ||
|
|
@@ -380,7 +386,8 @@ findAllocaInsertPoint(llvm::IRBuilderBase &builder, | |
| static llvm::Expected<llvm::BasicBlock *> convertOmpOpRegions( | ||
| Region ®ion, StringRef blockName, llvm::IRBuilderBase &builder, | ||
| LLVM::ModuleTranslation &moduleTranslation, | ||
| SmallVectorImpl<llvm::PHINode *> *continuationBlockPHIs = nullptr) { | ||
| SmallVectorImpl<llvm::PHINode *> *continuationBlockPHIs = nullptr, | ||
| bool saveFirstBlockForAlloca = false) { | ||
| llvm::BasicBlock *continuationBlock = | ||
| splitBB(builder, true, "omp.region.cont"); | ||
| llvm::BasicBlock *sourceBlock = builder.GetInsertBlock(); | ||
|
|
@@ -441,6 +448,14 @@ static llvm::Expected<llvm::BasicBlock *> convertOmpOpRegions( | |
| // Convert blocks one by one in topological order to ensure | ||
| // defs are converted before uses. | ||
| SetVector<Block *> blocks = getBlocksSortedByDominance(region); | ||
| llvm::BasicBlock *firstLLVMBB = moduleTranslation.lookupBlock(blocks.front()); | ||
| std::optional<LLVM::ModuleTranslation::SaveStack<OpenMPAllocaStackFrame>> | ||
| frame; | ||
|
|
||
| if (saveFirstBlockForAlloca) | ||
| frame.emplace(moduleTranslation, llvm::OpenMPIRBuilder::InsertPointTy( | ||
| firstLLVMBB, firstLLVMBB->end())); | ||
|
|
||
| for (Block *bb : blocks) { | ||
| llvm::BasicBlock *llvmBB = moduleTranslation.lookupBlock(bb); | ||
| // Retarget the branch of the entry block to the entry block of the | ||
|
|
@@ -2093,15 +2108,11 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder, | |
| LLVM::ModuleTranslation::SaveStack<OpenMPVarMappingStackFrame> mappingGuard( | ||
| moduleTranslation, reductionVariableMap); | ||
|
|
||
| // Save the alloca insertion point on ModuleTranslation stack for use in | ||
| // nested regions. | ||
| LLVM::ModuleTranslation::SaveStack<OpenMPAllocaStackFrame> frame( | ||
| moduleTranslation, allocaIP); | ||
|
|
||
| // ParallelOp has only one region associated with it. | ||
| builder.restoreIP(codeGenIP); | ||
| llvm::Expected<llvm::BasicBlock *> regionBlock = convertOmpOpRegions( | ||
| opInst.getRegion(), "omp.par.region", builder, moduleTranslation); | ||
| opInst.getRegion(), "omp.par.region", builder, moduleTranslation, | ||
| /*continuationBlockPHIs=*/nullptr, /*saveFirstBlockForAlloca=*/true); | ||
| if (!regionBlock) | ||
| return regionBlock.takeError(); | ||
|
|
||
|
|
@@ -2186,6 +2197,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder, | |
|
|
||
| llvm::OpenMPIRBuilder::InsertPointTy allocaIP = | ||
| findAllocaInsertPoint(builder, moduleTranslation); | ||
|
|
||
| llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder); | ||
|
|
||
| llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP = | ||
|
|
@@ -4022,7 +4034,8 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder, | |
|
|
||
| builder.restoreIP(codeGenIP); | ||
| llvm::Expected<llvm::BasicBlock *> exitBlock = convertOmpOpRegions( | ||
| targetRegion, "omp.target", builder, moduleTranslation); | ||
| targetRegion, "omp.target", builder, moduleTranslation, | ||
| /*continuationBlockPHIs=*/nullptr, /*saveFirstBlockForAlloca=*/true); | ||
|
|
||
| if (!exitBlock) | ||
| return exitBlock.takeError(); | ||
|
|
||
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.
TODO: Add new test(s) to reproduce the bug fixed by the PR once the reviewers agree with the taken approach.