[PATCH] D110173: [DebugInfo][InstrRef] Use correct (and existing) PHI placement utilities for machine locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

In D110173#3017893 <https://reviews.llvm.org/D110173#3017893>, @StephenTozer wrote:

> First pass looking through this, haven't gone through the unit test yet - I think I understand this roughly, and don't have any complaints, but will continue to review - 1 nit and 1 question enclosed.

Huzzah!,

Reviewing the whole set of unit tests might be beyond the call of duty, but would be appreciated,

I'm trying to piece back together what I'd intended in the unit tests, and it looks like it'd be clearer if the transfer function was completely re-constructed before each call to buildMLocValueMap. Right now, I'm relying on earlier state, which makes it harder to localise and understand a single block of test.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1756-1758
+    // If we've already eliminated a PHI here, do no further checking, just
+    // propagate the first live-in value into this block.
+    if (InLocs[Idx.asU64()] != ValueIDNum(MBB.getNumber(), 0, Idx)) {
----------------
StephenTozer wrote:
> My understanding of this is incomplete, under what circumstances would we have eliminated the PHI without having already assigned `InLocs[Idx.asU64()] = FirstVal`?
Great question; the elimination of one PHI might enable the elimination of additional PHIs, possibly higher up the CFG. That could cause the live-in value on entry to a block to change more than once. If we had for example:
               |
            loop head-------\
               |            ^
               A            ^
             /   \          ^
            /     \         ^  
          assign   B        ^ 
            \     /         ^    
             \   /          ^     
               C------------/
               |
             exit

Assume that we're dealing with $rax, which has some value prior to the loop head that I'll call "foo", and $rax is modified in the block labelled "assign". The PHI placement code isn't aware of what concrete values the register has, only that it's modified in the "assign" block. Therefore it installs two PHIs: at the 'C' block, and the loop head.

During value propagation, lets say that the "assign" block is found to not actually change the value of $rax: perhaps it modifies it, but then restores it from a stack spill slot. This means the PHI in block C can be eliminated: the live-in value is determined to be the PHI from "loop head"

Subsequently, we can identify that the PHI at "loop head" just feeds the value it generates in $rax back into itself on the backedge: that PHI can be eliminated too. We can then propagate the "foo" value into every block in the function. When we come to "C", there's no longer a PHI on entry to that block, but we still need to set "foo" as its live-in value for $rax. That's what this portion of code seeks to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110173