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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked an inline comment as done.
jmorse added a comment.

Good point about the EH_LABEL situation, that was indeed going to mess with things. Revising in a moment.

Will look at the extra constructor situation shortly.



================
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");
----------------
Orlando wrote:
> 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)
The purpose is signalling to SetInsertPoint that the location being set is at the start of the block, before any debug info -- if we pass a pure `Instruction *` then there's no way to store that information, wheras by passing an iterator, we can put a flag in the iterator to store this fact. (See: D153777).

As you say it's down to the semantics of the functions changing; I plan on linking to the general RFC thinymajig and having a terse summary. https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939


================
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());
 
----------------
Orlando wrote:
> Why change from `getFirstNonPHI` to `getFirstInsertionPt`? They are not equivalent - the latter also skips past EH instructions.
> 
> (This pattern is repeated in multiple places)
I've just been running into this elsewhere ._., all our code samples have exceptions disabled so we haven't hit this in the past. There is indeed a semantic difference between the two, so I'm revising these away and adding a `getFirstNonPHIIt` method that returns an iterator, again to signal whether a position is in front of debug-info or not. The relevant flag will be set at D153777

The result should be that when you get this:
    PHI
        <--- debug data
    ADD

Then getFirstNonPHIIt will return an iterator to the Add with the head-bit set, while if you have this:

    PHI
    EH_LABEL
        <--- debug data
    ADD

You'll get an iterator to the EH_LABEL, with the head bit set, but the bit won't have any effect because of the invariant that we don't insert debug-info before PHI's / LABEL's.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152468/new/

https://reviews.llvm.org/D152468