[PATCH] D46100: WIP: [IR/Verifier] Diagnose use-before-def scenarios for debug intrinsics

Wed Aug 22 17:50:06 PDT 2018

vsk added a comment.

In https://reviews.llvm.org/D46100#1210004, @bjope wrote:

> A problem with CodeGenPrepare::placeDbgValues is that it can move debug intrinsics more or less blindly, for example without taking other debug intrinsics refering to the same variable into account.
>  If there is a use-before-def scenario it often is correct to move the using dbg.value below the def, but that depends on which transform that resulted in such a situation (I guess that often it might be due to an opt-pass not caring about debug intrinsics).
>  Right now CodeGenPrepare::placeDbgValues also hoists dbg.value instructions when the use already is after the def. If I remember correctly it can even move a dbg.value from one basic block to another.

Right, but we can fix that by querying a dominator tree when deciding whether/not to move a dbg.value. Wdyt?

> We have TRs where we have noticed that debug-experience gets really weird due to CodeGenPrepare::placeDbgValues, because it has incorrectly moved dbg.value instructions.

I'd be happy to help take a look at these.

> (FWIW, the description in the beginning of CodeGenPrepare.cpp actually hints that the pass itself should be removed, that is however a different story and maybe that comment is out-dated, but I doubt that the original goal with that pass was that it should be a cleanup pass that tries to mend things that are broken elsewhere without having the knowledge about what the correct IR actually looked like)

CGP does enough work that I'm skeptical of it ever going away, even if LLVM ever transitions to using GlobalISel exclusively. I agree that it's not very principled to have a 'fix broken IR' pass, but the alternatives seem problematic in two ways: added complexity in the rest of the compiler, & the compile-time cost of figuring out where to place dbg.values every time a pass moves an instruction.

> I think that CodeGenPrepare::placeDbgValues exists for two different reasons:
> 
> 1. Bugs in IR passes introducing use-before-def scenarios (so if we remove the function we will lose some debug information that might (or might not) be valid today).

Yes, including bugs in CGP itself (see: r340370). If we remove placeDebugValues without some kind of replacement, variable availability will definitely regress.

> 2. Limitations in SelectionDAGBuilder, for example when it comes to properly handle "dangling" debug info between basic blocks, resulting in loss of debug information when the debug-use isn't in the same BB as def.

Right, there's an interaction between placeDebugValues and SelectionDAG. As I understand it, SelectionDAG's NodeMap only maps Values to SDNodes one basic block at a time. We keep track of dangling dbg.values in two cases: 1) the Value a dbg.value refers to may have been emitted in a previously-visited block, and 2) there is dbg.value use-before-def.

In case (1), it's unlikely (ideally impossible?) that the Value will be emitted again, so (IIUC) the dangling debug value will never be resolved. (If the value simply requires constant re-materialization it's handled in a separate path.) We might consider addressing this case by cloning the dbg.value into the previously-visited block: doing this reorders variable updates, but a sufficient increase in variable availability might make it a reasonable tradeoff.

In case (2), unless I'm missing something, incidences of dbg.value use-before-def can be eliminated in a relatively cheap way with a smarter placeDebugValues.

> The verifier check can help us with (1).
>  I've got some ongoing patches trying to improve (2), but unfortunately I haven't had time to continue on that work since before summer.
> 
> One thing that has been nagging me is in which order to do things. Removing CodeGenPrepare::placeDbgValues would be the main goal. But without first doing fixes in SelectionDAG it gives quite different results (getting rid of some faulty debug-info, but also losing some valid debug-info). If I do not remove CodeGenPrepare::placeDbgValues first, then it is harder to motivate patches in SelectionDAG, since the problems are hidden by placeDbgValues (test cases must start after CodeGenPrepare). And maybe we need to solve (1) and adding the verifier check before removing placeDbgValues, because I guess noone has verified that SelectionDAG can handle debug-use-before-def in a safe way.

https://reviews.llvm.org/D46100