[PATCH] D25347: [VirtRegRewriter] Eliminate COPYs before re-writing by renaming.

[Matthias Braun via llvm-commits]

MatzeB added a comment.

I just read the main function to understand the purpose, comments below.

So my understanding is that this looks for

  %vreg0 = COPY %vreg1

with vreg0 assigned different from vreg1 but where it is possible to use the vreg1's register for vreg0 as well without any interference?

This obviously needs to buildup new interference checking infrastructure. Why not do this as part of RegAlloc when LiveRegMatrix is still around?



================
Comment at: lib/CodeGen/VirtRegMap.cpp:393-394
+    // instruction.
+    if (!LI.containsOneValue())
+      continue;
+    const MachineInstr *MI =
----------------
Try `MachineRegisterInfo::hasOneDef()`, LiveRange::constainsOneValue() seems to also count invalid VNInfos and I am not sure those are all cleared here.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:401-405
+    unsigned DstReg = CopyDst.getReg();
+
+    // Only consider renaming virtual registers.
+    if (!TargetRegisterInfo::isVirtualRegister(DstReg))
+      continue;
----------------
Isn't that the same as VReg and should rather be `assert(CopyDst.getReg() == VReg);`


================
Comment at: lib/CodeGen/VirtRegMap.cpp:410-411
+
+    unsigned DstPhysReg = getPhysReg(*VRM, DstReg);
+    unsigned SrcPhysReg = getPhysReg(*VRM, SrcReg);
+
----------------
This needs to adjust to subregister indexes on the dest/source operand!


================
Comment at: lib/CodeGen/VirtRegMap.cpp:420
+
+    bool IsSrcConstant = !TargetRegisterInfo::isVirtualRegister(SrcReg) &&
+                         MRI->isConstantPhysReg(SrcPhysReg, *MF);
----------------
`!TargetRegisterInfo::isVirtualRegister` -> `isPhysicalRegister` that also avoids hitting NoReg (though not a problem in this case).


================
Comment at: lib/CodeGen/VirtRegMap.cpp:431
+    BlockFrequency Cost(0);
+    for (const MachineOperand &Opnd : MRI->reg_nodbg_operands(DstReg)) {
+      const MachineInstr &OpndInst = *Opnd.getParent();
----------------
Call it `MO` instead of `Opnd` like most of the code?


================
Comment at: lib/CodeGen/VirtRegMap.cpp:442
+        continue;
+      if (OpndInst.isFullCopy() && &OpndInst.getOperand(1) == &Opnd) {
+        BlockFrequency OpndFreq = MBFI->getBlockFreq(OpndInst.getParent());
----------------
Isn't `&OpndInst.getOperand(1) == &Opnd` always true and can be an assert?


================
Comment at: lib/CodeGen/VirtRegMap.cpp:444-447
+        unsigned OpndDstPhysReg =
+            getPhysReg(*VRM, OpndInst.getOperand(0).getReg());
+        unsigned OpndSrcPhysReg =
+            getPhysReg(*VRM, OpndInst.getOperand(1).getReg());
----------------
What about subregister indexes?


https://reviews.llvm.org/D25347