[PATCH] D152468: [NFC][DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

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