[PATCH] D138943: [WebAssembly] Use LiveDebugValues analysis

Tue Dec 20 08:30:22 PST 2022

jmorse added a comment.

> I removed all register-based (= dangling) DBG_VALUEs for Wasm in D139675 <https://reviews.llvm.org/D139675>; WebAssemblyDebugFixup runs right before this analysis. So now we don't have any virtual registers participating here, and I re-enabled the physical register assertion, so we can maintain LocIndex only deals with physical registers.

Excellent, that keeps the pass nice and simple,

> I'm working on (1) now. Would you mind if I address it as a follow-up child CL? I disabled LiveDebugValues analysis here so this not-yet-fully-implemented analysis doesn't run on Wasm pipeline, and we only check for my basic test cases here.

SGTM -- to state the obvious, it means that the analysis won't realise that over-written locals means the variable location should terminate, and they might be propagated into successor blocks. (And it should be fixed by the follow up).

In terms of this patch, I think it wants metadata node-number-capturing (see inline comment on test) but otherwise LGTM.

> By the way, may I ask a separate question? How should I handle defs in memory locations? Should I use SpillLocs? (They currently can have only registers as their base though) But looking at this code, it looks not every store instruction qualifies as a spill. Also you can access the same memory address even with a different register + offset, so it kind of looks all stores can terminate any indirect DBG_VALUEs. How do other targets handle defs in indirect DBG_VALUE locations?
>
> For Wasm, it looks we have a very small number of indirect DBG_VALUEs anyway, so maybe I can start with nullifying them and not dealing with them, as I did for DBG_VALUE_LISTs a while ago in D102999 <https://reviews.llvm.org/D102999>. But I'm generally curious about how memory locations should be handled. Sorry for many questions 😅

I'm very glad to answer debug-info questions -- by "defs in memory locations" do you mean that an instruction produces a value, not in a register, but at a memory location? There are some situations where this can be tracked on other targets, but it's messy, and we don't try to do it for heap memory because any pointer can alias it. There are two situations where memory transfers are tracked:

- For local stack spill slots, the LiveDebugVariables pass receives information during register allocation indicating where variable values are going, and indirect DBG_VALUEs are inserted whenever spills/restores occur.
- LiveDebugValues can recognise spill-and-restores instructions and transfer variables locations into spill slots, and back out, in case there are spills not recognised by LiveDebugVariables

As far as I understand it, the situation that LLVM handles very well is where (on x86 for example) a definition happens in a register that is then spilled, and then the spill is fused into the instruction, for example:

  $rax = addrr64 $rax, $rdx           <---- Some arithmetic instruction,
  MOV64mr $rsp, ???, 16, killed $rax  <---- A stack spill
  DBG_VALUE %stack.0, 0               <---- indirect DBG_VALUE inserted by LiveDebugVariables

Is fused into:

  addmr64 $rsp, ???, 16, killed $rdx  <---- Arithmetic adding into a stack slot,
  DBG_VALUE %stack.0, 0               <---- indirect DBG_VALUE inserted by LiveDebugVariables

I'm not aware of any other situation that LLVM can handle where a definition happens in memory -- and critically, the above code sequence is one where the memory location used to explicitly come from a spill instruction, but is then optimised further, so it's not a piece of information that is explicitly tracked. If that fusing happens during instruction selection and the value is defined into a regular local variable on the stack, then today it isn't be tracked.

Seeing how you've got a lot of control of the wasm pipeline, I imagine it's possible to add instrumentation and (perhaps) identify scenarios where definitions to stack-like memory happen and produce indirect DBG_VALUEs. That might be prohibitively complicated though.

This might be the point where I need to plug my re-designed variable tracking scheme, a summary is in this talk: https://www.youtube.com/watch?v=yxuwfNnp064 , you can find references to "instruction referencing" on the mailing list / discourse. The core idea is to number each instruction, and each operand of that instruction, connect variable information with those numbers and leave variable information in SSA form. Then at the end of compilation to re-analyse the code and find the machine locations for values. This means that you can describe a value as "The memory operand of instruction number $n" such as in this test [0]. The analysis at the end of codegen relies on being able to "see" all memory writes, so it too only works for stack slots. If there's something analogous in wasm then perhaps it could be fitted to that.

[0] https://github.com/llvm/llvm-project/blob/416fd03708d49b66900ef629c733fbbad2a602e2/llvm/test/DebugInfo/MIR/InstrRef/memory-operand-folding.mir

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
aheejin wrote:
> dschuff wrote:
> > aheejin wrote:
> > > dschuff wrote:
> > > > would it also be correct to condition this on MO.getReg() being a physical rather than a virtual reg? If so, I think something like that would be clearer than the more general condition that there can't be any vregs at all.
> > > Makes sense. Changed to check the register itself.
> > This change can be reverted now that we've removed the VRegs, right?
> Not really, at least not without touching other parts. This function is [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L2162-L2163 | called on all instructions ]], not only `DBG_VALUE`s, so our virtual registers enter here. They will not play any role in the rest of the pass though. Apparently `DefinedRegs` which are collected in this function is only used in [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L2165 | recordEntryValue ]] function, and we don't use [[ https://dwarfstd.org/ShowIssue.php?issue=100909.1 | entry values ]]. But anyway without this `if` we will crash.
> 
> On the other hand, [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L741-L745 | the other assertion ]] I had removed and revived, is in [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L728-L762 | VarLoc::insert ]], and only runs on registers in `DBG_VALUE`s, which we removed, so we can have that assertion.
This change won't hinder other targets, however you mention that virtual register don't participate in LiveDebugValues, so they don't need to be in the defined register collection AFAIUI. It might be cleaner to add the `isPhysicalRegister` condition to the `if` so that virtual registers are completely ignored.

================
Comment at: llvm/test/DebugInfo/WebAssembly/live-debug-values.mir:1-2
+# RUN: llc -run-pass livedebugvalues %s -o - | FileCheck %s
+
+# Test if LiveDebugValue analysis works on Wasm.
----------------
Could you make the CHECK lines a bit more robust by capturing the metadata number and comparing against that -- if you look at other debug-info tests for dbg.values you'll see they usually capture like:

    ![[VAR:[0-9+]]] = DILocalVariable(name: "foo"
    CHECK: dbg.value({{.*}}, ![[VAR]]...

or something. This saves us bother in the future if the IR module gets auto-upgraded, the metadata renumbers, and then the metadata node numbers don't match what's in the CHECK lines.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943