[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 marked 10 inline comments as done.
rastogishubham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2519
+    MachineInstr *DbgValueListInstr, MachineInstr *InstrToReplace) {
+  for (unsigned I = 2; I < DbgValueListInstr->getNumOperands(); I++) {
+    auto &Op = DbgValueListInstr->getOperand(I);
----------------
aprantl wrote:
> Why aren't you using `forEachDbgRegOperand()` here?
I missed this one


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2826
+          // RegisterMap that describes the DbgVar.
+          for (auto Reg : RegVec) {
+            auto RegIt = RegisterMap.find(Reg);
----------------
aprantl wrote:
> auto &?
Registers are just integers with helper functions, it doesn't matter either way, but sure


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2831
+            auto &InstrVec = RegIt->getSecond();
+            for (auto *It = InstrVec.begin(); It != InstrVec.end();) {
+              auto *I = *It;
----------------
aprantl wrote:
> does this loop get shorter if you implement it in terms of std::remove_if?
> 
> https://en.cppreference.com/w/cpp/algorithm/remove
It reduces it by 2 lines and for some reason it doesn't seem to work?


```
auto IsDbgVar = [&] (MachineInstr* I) -> bool {
              DebugVariable Var(I);
              return Var == DbgVar;
            };

            std::remove_if(InstrVec.begin(), InstrVec.end(), IsDbgVar);
```

Does that look right to you? It seems like remove_if doesn't work correctly with only one element?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2848
+          if (Op.isImm()) {
+            unsigned Opcode = Instr->getOpcode();
+            auto BI =
----------------
aprantl wrote:
> Would `Instr->getOperand(i).setReg(0)` work here and be shorter?
This will not work, the operand is an immediate and cannot be set to a $noreg by doing 
```
setReg()
```
the opcode of the operands are different and the only way to change it is to create a new instruction with a $noreg opcode for the first operand and erase the old Instr.


```
setReg()
```
calls 
```
getReg()
```
if the operand is an immediate that will assert


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2890
+        if (DbgIt != DbgValueSinkCandidates.end())
+          DbgValueSinkCandidates.erase(DbgIt);
+        // Zero out original dbg instr
----------------
aprantl wrote:
> Is it really necessary to look it up before erasing it, or can you call straight into erase, too?
Apparently not, when I do


```
DbgValueSinkCandidates.erase(DbgValueSinkCandidates.find(DbgVar));
```
or

```
auto DbgIt = DbgValueSinkCandidates.find(DbgVar);
DbgValueSinkCandidates.erase(DbgIt);
```

I get an assert


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

https://reviews.llvm.org/D145168