[llvm-dev] [RFC] Allowing debug intrinsics to reference multiple SSA Values

[Adrian Prantl via llvm-dev]

> On Feb 21, 2020, at 5:24 AM, Tozer, Stephen <stephen.tozer at sony.com> wrote:
> 
> What would it look like without this extension? If we modeled it as if all the register values were already on the stack (an extension of the current way where the singular value is modeled as being already on the stack, if I understand it correctly?)?
> 
> If it's decided that the best approach is to introduce something like DW_OP_LLVM_register - might be worth migrating to that first (basically adding DW_OP_LLVM_register(0) at the start of every DIExpression) and then expanding it from unary to n-ary support
> Putting the register values initially on the stack reduces the verbosity, though it could complicate successive salvages of variadic DIExpressions - if any value other than the last needs salvaging, then you have to use DWARF stack operations to move it to the top of the stack. For example, if the elements are pushed in order so that the last element is on the top of the stack:
> 
> %c = mul 3, %a
> %d = add 5, %b
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_plus), %c, %d)
> ; Salvage %d
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_plus_constu, 5, DW_OP_plus), %c, %b)
> ; Salvage %c needs to use DW_OP_swap
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_plus_constu, 5, DW_OP_swap, DW_OP_constu, 3, DW_OP_mul, DW_OP_plus), %a, %b)
> 
> Or, written out as the stack state:
> [%a, %b]               ; Initial state
> [%a, %b + 5]           ; DW_OP_plus_constu, 5
> [%b + 5, %a]           ; DW_OP_swap
> [%b + 5, %a, 3]        ; DW_OP_constu, 3
> [%b + 5, %a * 3]       ; DW_OP_mul
> [(%b + 5) + (%a * 3)]  ; DW_OP_plus
> 
> The simplest and most general solution would be to use DW_OP_pick, which duplicates the stack element at a given index to the top of the stack. This is more or less the same as using DW_OP_LLVM_register - both of them take an index corresponding to a register value - with a few differences: we get to maintain the default concise dbg.value with a single argument and empty DIExpression, but salvaging becomes more brittle. If we rely on elements being on the stack in their declared order, then we need to guarantee that nothing modifies those stack elements. The cleanest implementation of this would be for SalvageDebugInfo to salvage expressions normally when the last register is salvaged, and switch to using DW_OP_pick for every register whenever any other register is salvaged:
> 
> %c = add %a, 5
> %e = div %c, %d
> ; x = %b * ((%a + 5) / %d)
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_mul), %b, %e)
> ; Salvage %e; last operator so salvage normally
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_mul, DW_OP_div), %b, %c, %d)
> ; Salvage %c; not last operator and expression isn't already using pick, so add DW_OP_pick for registers
> dbg.value(!DILocalVariable("x"), !DIExpression(DW_OP_pick, 0, DW_OP_pick, 1, DW_OP_plus_constu, 5, DW_OP_pick, 2, DW_OP_div, DW_OP_mul), %b, %a, %d)
> 
> The major advantage of putting all registers on the stack is that it reduces verbosity and also doesn't require us to implement the new operator or update existing DIExpressions to contain it, which is useful. Personally I lean towards keeping things simple and consistent, and so using the pick/register operator in every expression rather than only the ones that need it, but there's a good case for either I think.


As the person who has advocated for DW_OP_LLVM_arg(N) before, my main motivation was to resolve the ambiguity of constant DIExpressions: As a worst-case example:

dbg.value(%undef, !DILocalVariable(x), DIExpression(DW_OP_constu, 42))

Is this undefined, or constant 42?

But if we make dbg.value fully variadic with all parameters pushed to the stack ahead of time, we can distinguish between

dbg.value(!DILocalVariable(x), DIExpression(DW_OP_constu, 42)) ; a constant
dbg.value(i32 42, !DILocalVariable(x), DIExpression()) ; the same constant
dbg.value(%undef, !DILocalVariable(x), DIExpression(DW_OP_constu, 42)) ; undef + garbage leftover expression

If we do it this way and allow 0-n SSA values then I'm in support of the push-first scheme.

As a side note, updating dbg.values in salvageDebugInfo when we allow multiple SSA values per dbg.value will be interesting, but possible. We'll need to put the value we need to update at the top of the stack, prepend the salvage expression and then restore the order of arguments on the stack that the rest of the expression expects.

-- adrian