[PATCH] D52634: [WebAssembly] Add DBG_VALUE with local operands location in WebAssemblyExplicitLocals pass

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

OK, I think we have enough out-of-tree experience now with this and the lldb side to expect that it will work, so we should really get it upstream (in this form or whatever form is acceptable).
So there are really 3 different elements to this patch.
1 is the DWARF expression type. I think we've found that this is preferable to repurposing `DW_OP_breg` as mentioned above. There was a question previously about whether this should be LLVM-specific or not. @yurydelendik I'm guessing that Mozilla's Rust DWARF library will be using this too, and LLVM won't be the only consumer, correct?
@aprantl Do you want to push back further or offer any other suggestions about this?

2 (the bulk of the patch) is all the target-independent infrastructure for using `TargetIndex` operands in  DbgValue-typed MachineInstrs and using that instead of a `MachineLocation` to express the location. Mechanically this seems straightforward to me; @aprantl do you have any opinions on that, or on my comment on lvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp?

3 is just the straightforward application of the previous 2 to the wasm backend.

This patch should perhaps be split into a target-independent part and a wasm-specific part; or at the very least it should be renamed, since the bit about WebAssemblyExplicitLocals is really the least consequential and most-straightforward part.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:583
+  LocationKind = Implicit;
+  emitOp(dwarf::DW_OP_WASM_location);
+  emitUnsigned(Index);
----------------
It's a bit odd that this is a target-independent extension but we directly encode the wasm-specific expression type here. Not sure if there's a better way or not.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:342
+  /// Emit location information expressed via target's index + offset
+  /// It is an extension for WebAssembly locals, globals and operand stack.
+  void addTargetIndexLocation(unsigned Index, int64_t Offset);
----------------
Since this is a generic extension and TargetIndex are usable by any target, how about saying something like "currently this is only used for WebAssembly locals..." etc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52634