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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for taking a look folks.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:5884-5886
       } else {
         return false;
       }
----------------
Orlando wrote:
> Orlando wrote:
> > This is unrelated to your change (but is debug-info related), sorry for the extra noise...
> > 
> > It looks like we early-out of `optimizePhiType` here if `II` has a user which doesn't match the conditions above (i.e. not a phi, store, or bitcast).  Couldn't this cause a change in behaviour based on debug-info if `II` is used by a debug intrinsic (`isa<DbgVariableIntrinsic>(V)`)?
> > 
> Ah, ignore this, I was being silly. I somehow forgot that debug intrinsics wrap the values as metadata.
Yep. This and other mulitple uses were what made me think this could cause a problem, but like you said it seems to be doing OK, both when I ran it like this with debugify and the original case with debug info.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:5959
   for (auto *I : DeletedInstrs) {
     I->replaceAllUsesWith(UndefValue::get(I->getType()));
     I->eraseFromParent();
----------------
bjope wrote:
> I guess this is what makes the dbg.value after the phi to be invalidated (getting undef as value instead of the phi result)?
> 
> Techincally I think it would have been OK to replace uses in dbg.value with the new PHI result (such as changing from i32 to float). At least salvageDebugInfoImpl in Local.cpp seem to just look through no-op-casts when. Maybe it isn't that important to deal with it here. I think it is more complicated than just calling salvageDebugInfo(*I) here, since it won't detect that you replaced the PHI with a new PHI. But maybe it is worth a comment explaining that we don't salvage any debug uses here (even thought it could be done in the future).
Oh of course. For some reason I was just thinking the undefs were from removing the node. I had not considered that we replace them with undef as we remove them, due to the cycles that can be found between phi nodes.

I have attempted to add some code to salvage debug info (largely as an excuse to learn a little more about debug info).

Let me know if it looks wonky.


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

https://reviews.llvm.org/D82678