[IR][NFC] Update IRBuilder to use InsertPosition #96497
Merged
Conversation
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
Uses the new InsertPosition class to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel.
SLTozer
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-compiler-rt-sanitizer @llvm/pr-subscribers-clang-codegen Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
|
@llvm/pr-subscribers-mlir Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
|
@llvm/pr-subscribers-backend-aarch64 Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
|
@llvm/pr-subscribers-backend-powerpc Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
|
@llvm/pr-subscribers-backend-arm Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
|
@llvm/pr-subscribers-backend-amdgpu Author: Stephen Tozer (SLTozer) ChangesUses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the Patch is 122.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96497.diff 97 Files Affected:
|
nikic
approved these changes
Jun 24, 2024
llvm/include/llvm/IR/Instruction.h
Outdated
| @@ -44,15 +44,19 @@ template <> struct ilist_alloc_traits<Instruction> { | |||
| iterator_range<simple_ilist<DbgRecord>::iterator> | |||
| getDbgRecordRange(DbgMarker *); | |||
|
|
|||
| /// Class used to generate an insert position (ultimately always a | |||
| /// BasicBlock::iterator, which it will implicitly convert to) from either: | |||
| /// - An Instruction, inserting immediately prior. | |||
There was a problem hiding this comment.
Indicate that this is deprecated?
There was a problem hiding this comment.
It isn't deprecated yet, but I can put a comment indicating the future intent. Or OTOH, while it's not marked LLVM_DEPRECATED, conceptually it's being/is deprecated - not sure if the terminology matters ("This is deprecated" vs "This will be deprecated").
| @@ -3136,7 +3136,7 @@ static void LLVMPositionBuilderImpl(IRBuilder<> *Builder, BasicBlock *Block, | |||
| Instruction *Instr, bool BeforeDbgRecords) { | |||
| BasicBlock::iterator I = Instr ? Instr->getIterator() : Block->end(); | |||
| I.setHeadBit(BeforeDbgRecords); | |||
| Builder->SetInsertPoint(Block, I); | |||
| Builder->SetInsertPoint(I); | |||
There was a problem hiding this comment.
Maybe assert that BB and Instr are consistent here?
| @@ -637,8 +637,7 @@ void ARMParallelDSP::InsertParallelMACs(Reduction &R) { | |||
| Intrinsic::getDeclaration(M, Intrinsic::arm_smlad) : | |||
| Intrinsic::getDeclaration(M, Intrinsic::arm_smlald); | |||
|
|
|||
| IRBuilder<NoFolder> Builder(InsertAfter->getParent(), | |||
| BasicBlock::iterator(InsertAfter)); | |||
| IRBuilder<NoFolder> Builder((BasicBlock::iterator(InsertAfter))); | |||
There was a problem hiding this comment.
Suggested change
| IRBuilder<NoFolder> Builder((BasicBlock::iterator(InsertAfter))); | |
| IRBuilder<NoFolder> Builder(BasicBlock::iterator(InsertAfter)); |
There was a problem hiding this comment.
Funnily enough, that doesn't actually work:
./llvm/lib/Target/ARM/ARMParallelDSP.cpp:640:32: warning: parentheses were disambiguated as a function declaration [-Wvexing-parse]
640 | IRBuilder<NoFolder> Builder(BasicBlock::iterator(InsertAfter));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hence the weird double parentheses - however having looked back at this I can see InsertAfter is always non-null, so ->getIterator() is safe.
SLTozer
added a commit
that referenced
this pull request
Jun 24, 2024
Reverts the above commit, as it updates a common header function and did not update all callsites: https://lab.llvm.org/buildbot/#/builders/29/builds/382 This reverts commit 6481dc5.
|
Looks like I missed a spot - reverting for now, will wait for a bit to see if any other errors fall out from the various configurations on CI before reapplying. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/979 Here is the relevant piece of the build log for the reference: |
|
Looks like using InsertPosition in IRBuilder has some overhead: https://llvm-compile-time-tracker.com/compare.php?from=317277e4f961edf13132914a58a26408db4ab0aa&to=6481dc57612671ebe77fe9c34214fba94e1b3b27&stat=instructions:u So it might make sense to keep some explicit overloads for IRBuilder (not sure which ones are hot though). |
Hm, we're adding an extra pointer chase when we give up passing a basicblock - I think then it makes sense to keep the option to pass BB+It, and do so when the caller already knows the BB (while removing the need to pass the BB around for functions that only have an instruction). |
My guess here would have been that the issue is the cases where an Instruction* or BasicBlock* is passed rather than BB+Iterator, because those two cases go through an out-of-line constructor. |
SLTozer
added a commit
to SLTozer/llvm-project
that referenced
this pull request
Jun 25, 2024
This reverts commit d75f9dd.
Seems like you're right - removing the extra dereference does little-to-nothing, but allowing the constructors to be inlined mitigates most of the cost. It still ends up being a net negative for performance though, even after allowing the constructors to become inlined. |
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Uses the new InsertPosition class (added in llvm#94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the `Instruction *InsertBefore` argument from instruction-creation (discussed [here](https://discourse.llvm.org/t/psa-instruction-constructors-changing-to-iterator-only-insertion/77845)), this will simplify the process by allowing us to deprecate the InsertPosition constructor directly and catch all the cases where we use instructions rather than iterators.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Reverts the above commit, as it updates a common header function and did not update all callsites: https://lab.llvm.org/buildbot/#/builders/29/builds/382 This reverts commit 6481dc5.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Uses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint.
This has no functional effect, but later on as we look to remove the
Instruction *InsertBeforeargument from instruction-creation (discussed here), this will simplify the process by allowing us to deprecate the InsertPosition constructor directly and catch all the cases where we use instructions rather than iterators.