[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:
> aheejin wrote:
> > 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?
> Yeah it's kind of weird because `VarLocBasedImpl` is technically target-neutral but has wasm-specific logic; I didn't think about not even being able to access the namespace and instruction names.
> But you did just call out the fundamental thing that separates the local-defs we care about from stack defs is that they have explicit operands. Maybe it's enough to call it `isExplicitTargetIndexDef` and for the blob comment to say that the intention is for this to cover explicit TargetIndex defs that can be tracked by their offset. (It's probably useful for the comment to say a bit more than just that, i.e. maybe also briefly describe the actual use of the function).
- Changed the function name to `isExplicitTargetIndexDef`.
- Expanded the comment a little more in `TargetInstrInfo.h`, and also added more detailed comments in this file (`WebAssemblyInstrInfo.cpp`), where the `WebAssemblyInstrInfo::isTargetIndexDef` is defined.
- In theory we need to add `global.set` here too, but we don't have global indices at this point because they are relocatable and we address them by names until linking, so we don't have 'offsets' (which are used to store local/global indices) to deal with in LiveDebugValues. And AFAIK we don't really store values that have debug info in wasm globals anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140373