How should I manage DBG_VALUEs when its def moves?

aheejin · January 11, 2023, 12:58am

Hello. I have some basic questions on how to manage DBG_VALUE instructions when moving def instructions.

This doc (Source Level Debugging with LLVM — LLVM 16.0.0git documentation) contains a little information on how to move DBG_VALUEs around when you move instructions within a BB. It mainly says when you hoist an instruction, leave its DBG_VALUE in its original place, and when you sink an instruction, you sink its DBG_VALUE alongside it but create a new DBG_VALUE $noreg in its original place. But it’s also confusing that some existing functions don’t seem to follow the rule, for example,
CodeGenPrepare::placeDbgValues in CodeGenPrepare.cpp, which seems to put DBG_VALUEs right after its corresponding register definition.

I am also wondering what I should generally do when I move an instruction across BBs. Should I mostly bring its DBG_VALUEs along? I think hoisting DBG_VALUEs with a def instruction can produce incorrect value combinations. But if I don’t hoist DBG_VALUEs, the register-defining instruction and its DBG_VALUEs can be separated across multiple BBs, and when the register-defining instruction later gets transformed, i.e., its register number changes, I don’t know how to reflect that transformation to its (now separated) DBG_VALUEs. Also there are cases, even right after instruction selection, in which DBG_VALUEs appear far from a def, or they even appear in a basic block where no definition for the register exists. When we try to figure out which DBG_VALUEs correspond to a certain instruction, we currently just grab DBG_VALUEs in the same BB that come after the instruction, but this misses DBG_VALUEs in other BBs.

I’d appreciate any help!

StephenTozer · January 11, 2023, 5:06pm

The short answer is that the Source Level Debugging document is correct; placeDbgValues and WebAssemblyDebugValueManager are both exceptions to those general rules. placeDbgValues is largely incorrect, and only still exists to serve as a last-minute backstop against dropping certain debug values. The WebAssembly example I’m not as familiar with, but I believe it is handling a specific case, making assumptions that do not hold for the general case. The principle behind the rules is that the position of debug values should be preserved, the order of debug values for a single variable (and fragment) should not be changed, and the lifespan of debug values should not be increased. This means that we don’t hoist, since it would result in a variable changing its value before the corresponding assignment takes place; we may sink a debug value to become live as soon as its value is available, but we do not sink it past other debug values for the same variable, and we leave an undef debug value behind to ensure that any prior debug value for that variable is terminated when the sunk debug value would have become live.

The general problem you refer to of tracking the registers/stack slots referred to by a debug value is relatively complicated, but the general approach is that after instruction selection, the values produced by instructions will be in virtual registers, which will not be overwritten or killed. Immediately after register allocation converts these to physical registers and stack slots, LiveDebugValues steps in and performs an analysis to track values moving through machine locations and/or being dropped, producing new debug values as needed after each move. In short, most passes prior to regalloc should generally be able to just follow the rules above, though as mentioned there are exceptions.

Hopefully this helps, but if you can give a more specific example of what you’re trying to do, then I’d be happy to answer/explain more specifically what’s happening and what should happen with debug values.

aheejin · January 12, 2023, 11:47pm

Thank you for the reply!

The WebAssembly example I’m not as familiar with, but I believe it is handling a specific case, making assumptions that do not hold for the general case.

To be clear, WebAssemblyDebugValueManager is something we wrote, and I’d like to fix it if that’s not correct. The reason I asked about it was I was not sure if that was the correct way to gather DBG_VALUEs associated with an instruction. You said it was an ‘exception’, then what would be a non-exceptional, or more conventional, way to do it? What assumptions does it have that do not hold for the general case?

The general problem you refer to of tracking the registers/stack slots referred to by a debug value is relatively complicated, but the general approach is that after instruction selection, the values produced by instructions will be in virtual registers, which will not be overwritten or killed. Immediately after register allocation converts these to physical registers and stack slots, LiveDebugValues steps in and performs an analysis to track values moving through machine locations and/or being dropped, producing new debug values as needed after each move. In short, most passes prior to regalloc should generally be able to just follow the rules above, though as mentioned there are exceptions.

Even in the virtual register + SSA mode, once a def is separated from its DBG_VALUEs, I’m not sure how to reconnect them or reflect the def’s changes to those DBG_VALUEs. Even if a register is only set once, I don’t think I can guarantee that in the BB that DBG_VALUEs exist, that register has been set on all paths leading up to that BB. I’ll give an example below.

Hopefully this helps, but if you can give a more specific example of what you’re trying to do, then I’d be happy to answer/explain more specifically what’s happening and what should happen with debug values.

For example, if there’s a BB like this,

bb.1:
  …
  %5 = SOME INST
  DBG_VALUE %5, …
  DBG_VALUE %5, …

And after some optimization, the %5’s defining instruction is hoisted to another BB:

pre:
  …
  %5 = SOME INST
  …

bb.1:
  …
  DBG_VALUE %5, …
  DBG_VALUE %5, …
  …

As we talked about, those DBG_VALUEs are not supposed to be hoisted with SOME INST. Now %5’s definition and its DBG_VALUEs are separated across BBs. Then in a later pass, if we want to change %5 = SOME INST’s register to %8, I don’t think we can change its DBG_VALUE %5s, which are now in bb.1, to DBG_VALUE %8, because there can be a path to bb.1 that doesn’t encounter pre BB and we can’t guarantee %8’s value always contains the same thing in all paths leading up to bb.1.

This is what an optimization can cause, but this kind of “separated” def and DBG_VALUE happen even right after instruction selection, because the bitcode is already that way. One example I found from a small program is, before instruction selection, this llvm.dbg.value is not in the same BB as its def in the first place, and it’s hard to even pinpoint which instruction is its corresponding def:

if.end19:                                         ; preds = %if.then17, %for.body
  %conv.i36 = and i32 %c.044, 255, !dbg !118                                     
  call void @llvm.dbg.value(metadata i32 %c.044, metadata !88, metadata !DIExpression()), !dbg !121
  …

After instruction selection, this becomes DBG_VALUE %6, and we don’t know where that %6’s def is:

bb.12.if.end19:                                                                  
; predecessors: %bb.10, %bb.11                                                   
  successors: %bb.14(0x04000000), %bb.13(0x7c000000); %bb.14(3.12%), %bb.13(96.88%) 
                                                                                 
  %48:i32 = CONST_I32 255, implicit-def dead $arguments                          
  %49:i32 = AND_I32 %6:i32, killed %48:i32, implicit-def dead $arguments, debug-location !120; wc.c:67:19 @[ wc.c:100:10 ]
  DBG_VALUE %6:i32, $noreg, !"c", !DIExpression(), debug-location !123; wc.c:0 @[ wc.c:100:10 ] line no:65
  …

So in these cases, once DBG_VALUEs are separated (sometimes rightfully, as in case of hoisting) from its def, I’m not sure how to reflect changes to the def to those DBG_VALUEs.

StephenTozer · January 16, 2023, 5:13pm

To be clear, WebAssemblyDebugValueManager is something we wrote, and I’d like to fix it if that’s not correct. The reason I asked about it was I was not sure if that was the correct way to gather DBG_VALUEs associated with an instruction. You said it was an ‘exception’, then what would be a non-exceptional, or more conventional, way to do it? What assumptions does it have that do not hold for the general case?

The assumption that it appeared to be making is that it is operating on simple stack-allocated variables, which are represented by llvm.dbg.declare intrinsics in IR and are typically the only non-global debug values in -O0 -g builds. In those cases, it is fine to have DBG_VALUEs immediately follow their defining operation, because we expect the variable to be visible as soon as it is allocated, and its stack slot will contain the variable’s value for the entire scope of the variable. In optimized debug builds (e.g. -O2 -g) however it is more common to see variables have many DBG_VALUEs within their scope, and the position at which a variable is assigned a new value in the source is not guaranteed to be at the same position that that value is produced.

Even prior to instruction selection a debug value may be separated from the instruction(s) that define its value, but it should still be dominated by them - the IR transformations preserve this, and ISel ensures it via placeDbgValues. CodeGen transformations also generally attempt to preserve the correctness of DBG_VALUES, by ensuring that lifetimes begin and end at the correct points and that they refer to the correct value. The question of how you should update a DBG_VALUE after its def is transformed is best answered on a pass-by-pass basis. To take your example of a vreg %5 being in a separate block to its corresponding DBG_VALUEs: if every pass has updated debug info correctly then %5 should dominate that DBG_VALUE, meaning it will have an available value on every path to the DBG_VALUE. If %5 is then replaced by %8, the correct method would depend on what the pass that performed the replacement was doing:

If the definition of %8 happens immediately after %5, or within the same block, then you can trivially assume that all DBG_VALUEs following the definition of %8 that referred to %5 should now refer to %8, applying any necessary transformations (for example if %8 is a pointer to a stack location that now contains the value of %5, you would update the DBG_VALUE to dereference %8 and get to the value it contains).
If the definition of %8 happens in another block that does not dominate all the DBG_VALUE uses of %5, then you would update DBG_VALUEs the same as other instructions - if the pass is iterating through instructions in one or more blocks and updating uses of %5 -> %8, then you would do the same for any DBG_VALUEs encountered; if there is a register that joins %5 and %8 at the block where their definitions meet, then you would use that value as you would for any other instructions in that block; if it is not clear how a given DBG_VALUE could be updated, or if providing the correct value would require inserting some new instruction, then the DBG_VALUE should be set as “undef”, as it is effectively optimized out.

There will be some cases however in which point 2 is difficult to implement. In some cases it may be obvious what value a DBG_VALUE needs to be assigned; if it is not immediately obvious what value a DBG_VALUE needs to be assigned however, then determining the correct value requires you to know the dominating defining instruction for each DBG_VALUE, which is quite expensive to calculate. This means that in some places we unfortunately either conservatively drop DBG_VALUEs or update them naively, potentially resulting in incorrect values.

This problem is largely solved by using a different instruction, DBG_INSTR_REF, which does not refer to virtual registers but instead carries a direct reference to the instruction that defines it. This means that it does not need to be sunk or updated alongside its def instruction; it simply keeps a reference to that instruction, and then the LiveDebugValues pass performs a dataflow analysis to determine what register/stack slot contains the desired value at each place that a DBG_VALUE wants to use it. If possible, implementing/enabling DBG_INSTR_REF for your target might be the best way to solve this problem; DBG_INSTR_REF is currently enabled by default for x86_64 only, but can be switched on by passing -Xclang -fexperimental-debug-variable-locations to clang or -fexperimental-debug-variable-locations to LLVM tools - it does not intrinsically require x86_64, but it hasn’t been fully tested on other architectures. The logic is mostly target-independent and so it may work out-of-the-box for Web Assembly, but if it does not then it may still be worth adding support, as it removes the need to perform any kind of dominance analysis prior to the LiveDebugValues pass.

jmorse · January 17, 2023, 10:32am

Just to add some specifics to what Stephen said, LiveDebugVariables will perform a liveness query and drop any DBG_VALUEs that are out of liveness before the register allocator runs. That means debug users that aren’t dominated by the definition of their operand should ™ be safely dropped.

The need for dominance information when updating DBG_VALUEs after we leave SSA form was the major motivator for instruction referencing, it effectively keeps debug-info in SSA form until the end of compilation, then computes the dominance information once on the final state.

aheejin · January 19, 2023, 6:36am

@StephenTozer @jmorse Thanks a lot for the answers!

Wasm uses virtual registers like physical registers, meaning, we don’t go through the register allocation process but simply remove PHIs at some point and allow multiple definitions to a single virtual register. (Wasm is a stack machine and those (non-SSA) virtual registers will be removed at the very end in AsmPrinter to emit instructions in a correct stack push-pop order. But this is not relevant here)

And most of Wasm passes are after we are out of SSA form, tracking DBG_VALUEs harder. Reading your answers, I think it wouldn’t be trivial to do better than we currently do, which only takes care of DBG_VALUEs occurring after a def within the same BB, because without dominator analysis it’s hard to precisely connect DBG_VALUEs to defs across BBs.

All this makes DBG_INSTR_REF’s motivation very compelling. I have some questions though:

How much scope byte coverage does it increase compared to the DBG_VALUE approach? This talk (2021 LLVM Dev Mtg “Improving debug variable location coverage by using even more SSA” - YouTube) said it improved the scope byte coverage by ~3%. Is this still the case or has it improved more since then? If not, is 3% tangible enough in real debugging experience?
To use it for Wasm, I think we need LiveDebugValues/InstrRefBasedImpl.cpp support for Wasm. (I recently committed Wasm support for LiveDebugValues/VarLocBasedImpl.cpp) What else do you think we need? Can we mostly reuse our handling code for DBG_VALUE for DBG_INSTR_REF? Or do we not need to move DBG_INSTR_REF at all when we move defs? (Even when the defs are sunk?)

Sorry for many questions again And thank you!

StephenTozer · January 19, 2023, 12:20pm

The use of virtual registers as physical registers may make it more complicated to correctly track registers in DBG_VALUES, although from the sounds of it up until you remove PHIs and start allowing multiple assignments to virtual registers the existing passes should be able to update DBG_VALUEs as normal. AFAIK very few transformations that would affect DBG_VALUEs run after register allocation and LiveDebugValues normally - if the same is true for WASM’s de-PHI-ing pass, then I’d imagine your results wouldn’t be much worse than for other targets, since the pre-PHI-removal passes would still be dealing with single-assignment registers…

In terms of adding DBG_INSTR_REF support for WASM, the LDV implementation might not currently be suited to handling virtual registers, since it assumes (and currently requires) there are only physical registers being used by that point. I don’t think the logic is strictly tied to the registers being physical though, so it may not be a difficult fix - the InstrRefBasedImpl (similar to the VarLocBasedImpl) mostly uses IDs that map to machine locations rather than referring to them directly in most places. And you don’t need to move DBG_INSTR_REFs at all - part of the intent is that if possible, we never move debug instructions (not counting operations that move their entire containing block); since DBG_INSTR_REFs will refer to the same instruction regardless of where that instruction is sunk or hoisted to, there is no need to move them at all.

With regards to scope byte coverage - I don’t have up-to-date numbers to-hand, but scope byte coverage isn’t the only factor to consider; one of the important features of DBG_INSTR_REF is that it does a lot of work to prevent incorrect variable locations from being produced by classes that reorder instructions or rewrite register uses, such as MachineSink or RegisterCoalescer. On some projects you could potentially see (and in practice we have seen) the scope byte coverage decrease if there are more incorrect locations than missing valid locations.

aheejin · January 21, 2023, 6:17pm

Thank you! I’ll take a closer look on DBG_INSTR_REF. I may end up asking more questions on it on the board…