[PATCH] D140373: [WebAssembly][LiveDebugValues] Handle target index defs

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp:208
+
+bool WebAssemblyInstrInfo::isTargetIndexDef(const MachineInstr &MI, int &Index,
+                                            int64_t &Offset) const {
----------------
dschuff wrote:
> Technically this function is "wrong" because stack value defs and global.sets are also target index defs. Your commit message explains why we only care about local defs and I agree that's the behavior we want; but it is a bit confusing.
> One option would be to implement the "correct" behavior here (i.e. return true for all of those), and then filter out the non-local ones in the caller. But that would be more code and slower for no good reason. The other option would be to change the name of the function to better reflect what we want it to do, but because it's technically target-independent I don't know of a good name. Another option would be to keep the name and behavior the same but put a big comment somewhere with an explanation. None of those sound particularly great, or particularly catastrophic. Thoughts?
Doing something in the caller doesn't work because `VarLocBasedImpl.cpp` is under `lib/CodeGen` and it cannot access `WebAssembly::` namespace or any of its instruction names such as `WebAssembly::LOCAL_SET` there. We need `lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h` to access them, and this file is only accessible within `lib/Target/WebAassembly`. This the reason I added this utility function here in `TargetInstrInfo` in the first place.

Also, implementing the "correct" behavior means returning true for all instructions with a def, because they produce stack values, which I guess is more than half of Wasm instructions anyway so there's no point of doing that. Also what we want here is a target index and an operand, which only `local.***` instructions have.

That leaves us only with the option of changing the function name or putting a big comment blob or both. Do you have any suggestions for a better name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140373