[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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

I think I'm happy with how the code looks now, maybe wait a few days to see if @jmorse has any more comments.



================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2783
+  SmallDenseMap<MachineInstr *, SmallVector<Register>, 8> InstrMap;
+  for (; MBBI != E; ++MBBI) {
+    MachineInstr &MI = *MBBI;
----------------
This might make it easier to see what this look is doing:
```
for (auto MBBI = MBB->begin(),  E = MBB->end(); MBBI != E; ++MBBI)
```



================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2873
+      for (auto *DbgInstr : DbgInstrVec) {
+        MachineBasicBlock::iterator InsertPos = MBBI;
+        InsertPos++;
----------------
You could combine these two lines as:
```
MachineBasicblock::iterator InsertPos = std::next(MBBI);
```


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2848
+          if (Op.isImm()) {
+            unsigned Opcode = Instr->getOpcode();
+            auto BI =
----------------
rastogishubham wrote:
> 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
Too bad. I would probably factor out a replaceOperand helper here to keep the main algorithm as readable as possible.


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

https://reviews.llvm.org/D145168