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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1650
+  // Find the target indices killed by MI, and delete those variable locations
+  // fromthe open range.
+  VarLocsInRange KillSet;
----------------



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp:208
+
+bool WebAssemblyInstrInfo::isTargetIndexDef(const MachineInstr &MI, int &Index,
+                                            int64_t &Offset) const {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140373