[CGP] Set debug locations when optimizing phi types

[dmgreen]

Summary

This sets the debug location from the old instructions when converting phi types.

The debug values may be more difficult to correct given that they are now a different type. Otherwise, I do not believe that debug instructions alters the optimization.

[dmgreen]

Rebase. I don't claim to be a big debug expert, I'm not sure if the debug value on bitcasts/phis is super important.

[SjoerdMeijer]

I am also not a debug expert, but this looks like an "innocent" patch to me that makes things a bit better, so that's good. What I am wondering about though why setDebugLoc isn't done in the constructor, which makes the code cleaner here and also it won't be forgotten. But I don't want to make this bigger than it is, and since I have never really looked into debug info, I also don't know if there would be any disadvantages doing that. Perhaps others can comment on that.

[bjope]

Just want to clarify that my questions regarding "what about debug info" in the earlier patch introducing these phi-type-rewrites actually was about dbg.value rather than the debugloc:s. I kind of wanted to have test cases verifying that the the presence of dbg.value uses didn't impact the optimization (-g invariance). But also to see that the dbg.value uses were handled somehow (either being salvaged in some way or being properly invalidated).

Nevertheless, if we should focus on the DebugLoc:s here, the patch seems pretty harmless to me. There are often some different opinions about having debug locations on things that doesn't really map to something in the original source. Afaik not having debug locations could mess up stepping in a debugger a bit, while having locations could impact profiling and code coverage. As far as I can see you pick locations from the replaced och adjacent instruction, so you won't end up hoisting/sinking a location risking to impact code coverage.

[bjope]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5970

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).

[Orlando]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5884–5886

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))?

[Orlando]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5884–5886

Ah, ignore this, I was being silly. I somehow forgot that debug intrinsics wrap the values as metadata.

[dmgreen]

Thanks for taking a look folks.

llvm/lib/CodeGen/CodeGenPrepare.cpp

5884–5886

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.

5970

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.

[jmorse]

This completely fell off my radar sorry,

This looks good, particularly the extra salvaging. A question inline about whether some of it is already covered by a RAUW though.

llvm/lib/CodeGen/CodeGenPrepare.cpp

5925–5933

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.

[dmgreen]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5925–5933

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.

[djtodoro]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5908

I think this should be a separate patch with the test case covering this.

[jmorse]

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

llvm/lib/CodeGen/CodeGenPrepare.cpp

5925–5933

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.