[PATCH] D47369: [DebugInfo][ScheduleDAGInstrs] Prevent scheduler from reordering DBG_VALUE instructions through their clobbering MIs

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Is this only an issue with AArch64, or is this more generic? Have you seen how this applies to other targets (say X86)?



================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:76-78
+static cl::opt<bool> EnableDbgValueReattach("enable-dbg-value-reattach",
+    cl::Hidden, cl::init(true),
+    cl::desc("Enable dbg_value reattachment to the nearest clobbering MI"));
----------------
Is there value in keeping this hidden under a flag? What is the risk of just doing this by default?


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:699
+// In the other case, returns 0.
+static unsigned isDescribedByReg(const MachineInstr &MI) {
+  assert(MI.isDebugValue());
----------------
Name is a bit misleading -- `isDescribedByReg` sounds like it should return a bool. This might be better named as `getRegDescriptor` or `regDescriptor` instead. Then you can describe this as returning the register number.


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:701
+  assert(MI.isDebugValue());
+  assert(MI.getNumOperands() == 4);
+  // If location of variable is described using a register (directly or
----------------
I must be missing something, but why is this assertion required here?


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:718-720
+    if (unsigned reg1 = isDescribedByReg(MI))
+      if (unsigned reg2 = isDescribedByReg(dbgMI))
+      return reg1 == reg2;
----------------
Is this a formatting error, is there something else missing here to catch the case where `reg1 != 0 && reg2 == 0`?


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:721-722
+      return reg1 == reg2;
+  }
+  else {
+    if (MI.mayStore())
----------------
Looks weird, maybe should be:

```
} else {
```

instead?


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:816
   // Walk the list of instructions, from bottom moving up.
-  MachineInstr *DbgMI = nullptr;
+  // Registered dbg_value instructions and their index in DbgValues vector
+  std::list<std::pair<MachineInstr*, unsigned>> DbgMIs;
----------------
s/Registered/Track/ ?

also:

s/index/indices/ ?


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:836
+    if (LastDbgMI) {
+      DbgValues.push_back(std::make_pair(LastDbgMI, &MI));
+      if (reattachDbgValueToNearestClobbering &&
----------------
How about using `emplace` instead?


Repository:
  rL LLVM

https://reviews.llvm.org/D47369