[PATCH] D138943: [WebAssembly] Use LiveDebugValues analysis

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Oh, I found you suggested to add comments here in D140373 <https://reviews.llvm.org/D140373>.

In D140373#4016029 <https://reviews.llvm.org/D140373#4016029>, @jmorse wrote:

> Thanks for assurance about the stack-popping being addressed elsewhere -- it might be worth putting a comment in D138943 <https://reviews.llvm.org/D138943> next to the definition of struct WasmLoc about that being a deliberate non-objective, to enlighten future readers.

I tried that, but I'm not sure if this is the best location to put this kind of target-specific comment. This is what I've tried:

  --- a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
  +++ b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
  @@ -306,7 +306,12 @@ private:
   
       // Target indices used for wasm-specific locations.
       struct WasmLoc {
  -      int Index; // One of TargetIndex values defined in WebAssembly.h
  +      // One of TargetIndex values defined in WebAssembly.h. We deal with
  +      // local-related TargetIndex in this analysis (TI_LOCAL and
  +      // TI_LOCAL_INDIRECT). Stack operands (TI_OPERAND_STACK) will be handled
  +      // separately WebAssemblyDebugFixup pass, and we don't associate debug
  +      // info with values in global operands (TI_GLOBAL_RELOC) at the moment.
  +      int Index;
         int64_t Offset;
         bool operator==(const WasmLoc &Other) const {
           return Index == Other.Index && Offset == Other.Offset;

This is basically the copy of what I said in `WebAssemblyInstrInfo.cpp` in D140373 <https://reviews.llvm.org/D140373>. That file is within `lib/Target/WebAssembly`, so I feel like I can elaborate on as many target-specific things as I want. But I'm not so sure about `VarLocBasedImpl.cpp`, which is target-independent, and especially when we don't need to distinguish anything about local and stack operands within that file. (We treat evert target index in the same way in the file; the part that handles the difference is encapsulated within

  if (!TII->isExplicitTargetIndexDef(MI, Index, Offset))                         
    return;

which has its own detailed comments in `WebAssemblyInstrInfo.cpp`. This part is added in D140373 <https://reviews.llvm.org/D140373>; it's not here yet.

I haven't committed this diff yet, because of this concern. Please let me know if you think this is a good idea there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943