[PATCH] D56401: [WebAssembly] Fix updating/moving DBG_VALUEs in RegStackify

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

I haven't finished reading other parts, but I think I should understand this `AttachedDebugValues` constructor first to proceed.



================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:490
+    for (auto &Op : MRI.reg_operands(Reg)) {
+      if (Op.isDef() && Defs.find(Op.getParent()) == Defs.end())
+        Defs.insert(Op.getParent());
----------------
`Defs` is a set. Why do we need to test if it already exists in the set when we add it?


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:477
 
-static void MoveDebugValues(unsigned Reg, MachineInstr *Insert,
-                            MachineBasicBlock &MBB, MachineRegisterInfo &MRI) {
-  for (auto &Op : MRI.reg_operands(Reg)) {
-    MachineInstr *MI = Op.getParent();
-    assert(MI != nullptr);
-    if (MI->isDebugValue() && MI->getParent() == &MBB)
-      MBB.splice(Insert, &MBB, MI);
+class AttachedDebugValues {
+  unsigned OpReg;
----------------
Please move this class to anonymous namespace above, because now this is in the global namespace


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:492
+        Defs.insert(Op.getParent());
+    }
+
----------------
By the way the loop can be replaced with `MRI->def_instructions(Reg)`


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:494
+
+    for (auto &Op : MRI.reg_operands(Reg)) {
+      MachineInstr *MI = Op.getParent();
----------------
It looks like you assume `Op` is a use and not a def in this loop, right? Maybe you should use not `reg_operands` but `def_operands`.


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:498
+      if (!MI->isDebugValue() || MI->getParent() != &MBB)
+        continue;
+      // Scan backwards to find very next operand with isDef set. If instruction
----------------
So by this point it looks like you assume `Op` is a use and not a def, right? Maybe you should also add a clause to exclude defs as well, like
```
if (Op.isDef())
  continue;
```


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:499
+        continue;
+      // Scan backwards to find very next operand with isDef set. If instruction
+      // is the Def, then the DebugValue is assotiated with it.
----------------
What is `isDef` set? Maybe you mean `Defs` set?


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:500
+      // Scan backwards to find very next operand with isDef set. If instruction
+      // is the Def, then the DebugValue is assotiated with it.
+      bool FoundDef = false;
----------------
associated


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:502
+      bool FoundDef = false;
+      for (MachineBasicBlock::reverse_instr_iterator
+               I = std::next(MI->getReverseIterator()),
----------------
Maybe we can use `auto` here?


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:506
+           I != E; ++I) {
+        if (Defs.find(&*I) == Defs.end())
+          continue;
----------------
You can use `Defs.count(&*I)` to check for an element's existence


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:514
+        Operands.push_back(&Op);
+        if (Instrs.find(MI) == Instrs.end())
+          Instrs.insert(MI);
----------------
`Instrs` is a set. Why do we need to test if it already exists in the set when we add it?


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:517
+      }
+    }
   }
----------------
Sorry, but I can't understand what this loop is doing. I thought what we were supposed to do to fix the 2nd point in [[ https://bugs.llvm.org/show_bug.cgi?id=40209 | PR40209 ]] was we needed to figure out which `DBG_VALUE` instruction was associated with which def, in case there were multiple defs to a register within a BB.

What this loop seems to be doing is gather all use operands into `Operands` and get all defining instructions into `Instrs`, and I can't figure out how we are gonna figure out the mapping between MI and `DBG_VALUE`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56401