[PATCH] D145168: Move DBG_VALUE's that depend on loads to after a load if the load is moved due to the pre register allocation ld/st optimization pass

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

rastogishubham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2607
+  // inst_b
+  // DBG_VALUE %2, "x", ...
+  // %2 = ld ...
----------------
aprantl wrote:
> // DBG_VALUE undef, "x"
This is what would happen if we didn't do what we are doing in the patch. The DBG_VALUE would remain untouched


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2760
+  E = MBB->end();
+  DenseMap<const DILocalVariable *, MachineInstr *> LocalVarMap;
+  DenseMap<MachineInstr *, Register> InstrMap;
----------------
aprantl wrote:
> aprantl wrote:
> > Might consider a `SmallDenseMap<8>` for better performance in the average case.
> > If you want to be really scientific about it you could compile a large file with optimizations and print the LocalVarMap.size() at the end of the function and then see if there is a sweet spot of small element size (<16) based on that data. But using 8 is fine, too!
> Would a name like DbgValueSinkCandidates be more descriptive? (Not sure if that captures the semantics perfectly).
I think that name makes sense


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2764
+    MachineInstr &MI = *MBBI;
+    if (MI.isDebugInstr() && MI.isDebugValueLike()) {
+      auto Op = MI.getOperand(0);
----------------
jmorse wrote:
> aprantl wrote:
> > Are there isDebugInstr() that are not isDebugValueLike()?
> > 
> > 
> DBG_LABEL, I believe -- isDebugValueLike should be sufficient.
We do not need to move DBG_INSTR_REFS, so I am going to use 
```
isDebugValue()
```
instead.




================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2767-2770
+      // TODO: This should not happen, have to fix the MIR verifier to check for
+      // such instances and fix them.
+      if (!DbgVar)
+        continue;
----------------
jmorse wrote:
> I think there are some tests out there that deliberately give an empty variable to test things like the the MIR parser. IMHO, asserting here is acceptable as any "real" DBG_VALUE must have a variable.
Unfortunately that is not the case, when I tried to build compiler-rt with this patch, the assert 
```
assert(DbgVar)
```
tripped. There is something that is letting DBG_VALUEs have no variable in them slip. I will look into it in a future patch


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2807
+        MBB->splice(InsertPos, MBB, DbgVal);
+        MBBI++;
+      }
----------------
aprantl wrote:
> Is this safe? Aren't we going to visit the instruction we just inserted next? I'm not sure how the semantics of the iterator are defined if inserting on the fly.
This is safe, the MBBI is incremented again in the for loop, and this is the end of the loop.


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

https://reviews.llvm.org/D145168