[PATCH] D82678: [CGP] Set debug locations when optimizing phi types

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

The variable location changes LGTM, I otherwise defer to Djordje who's made a request.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:5925-5933
+    SmallVector<DbgValueInst *, 1> DbgValues;
+    findDbgValues(DbgValues, Phi);
+    for (DbgValueInst *DVI : DbgValues) {
+      // Bitcasts are apparently irrelevant for debug info. Rewrite the dbg.value
+      // to use the new phi.
+      DVI->setOperand(0, MetadataAsValue::get(Phi->getContext(),
+                                              ValueAsMetadata::get(NewPhi)));
----------------
dmgreen wrote:
> jmorse wrote:
> > I'm not overly familiar with this optimisation, but won't the debug users of Phi be RAUW'd to the bitcast by the next loop? If so, there Shouldn't (TM) be a need to explicitly point them at the new PHI.
> The offending part is down in CodeGenPrepare::optimizePhiTypes where we call I->replaceAllUsesWith(UndefValue::get(I->getType())).  Because we are dealing with a collection of phi nodes, that may form a loop, we need to ensure they really have no users before we erase them.
> 
> And we are converting the type of the phis from float to int, so don't RAUW on them directly. We add new phi nodes and remove the old.
> 
> I'm certainly not a debug expert and if this code is going to cause more trouble that it's worth we can remove it. But I think (unconfidently) that it's needed to keep the debug values present and connected.
Cool -- the test changes look like they're doing the right thing, and it doesn't look like the other added call to salvageDebugInfo is responsible, so this block is making a good contribution.


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

https://reviews.llvm.org/D82678