[PATCH] D152468: [NFC][DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes
Orlando Cazalet-Hyams via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 24 06:15:26 PDT 2023
Orlando added a comment.
Herald added a subscriber: wangpc.
The changes from `getFirstNonPHI` to `getFirstInsertionPt` make me worry that this isn't NFC as it looks like the latter skips past EH instructions.
================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2198
// or the bit width of the operand.
- Builder.SetInsertPoint(&EndBlock->front());
+ Builder.SetInsertPoint(EndBlock, EndBlock->begin());
PHINode *PN = Builder.CreatePHI(Ty, 2, "ctz");
----------------
IIUC `front` and `begin` are equivalent (at the moment). What is the purpose of this change? If it's set-up for a future change in semantics of these functions it could be worth explaining the change in the commit message IMO.
(This pattern is repeated in multiple places)
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:730
// Need a final PHI to reconverge to above the helper lane branch mask.
- B.SetInsertPoint(PixelExitBB->getFirstNonPHI());
+ B.SetInsertPoint(PixelExitBB, PixelExitBB->getFirstInsertionPt());
----------------
Why change from `getFirstNonPHI` to `getFirstInsertionPt`? They are not equivalent - the latter also skips past EH instructions.
(This pattern is repeated in multiple places)
================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:1720
WidePhi->addIncoming(DU.WideDef, UsePhi->getIncomingBlock(0));
IRBuilder<> Builder(&*WidePhi->getParent()->getFirstInsertionPt());
+ Builder.SetInsertPoint(WidePhi->getParent(),
----------------
nit: Possibly redundant ctor (insertion point set below now) - possibly clearer to use the `Context &` one (with other default params)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152468/new/
https://reviews.llvm.org/D152468
More information about the llvm-commits
mailing list