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

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:158
+// Map from physical regunit to list of def SlotIndexes.
+using PhysDefsMap = DenseMap<unsigned, SmallVector<SlotIndex, 4>>;
+
----------------
For consistency with the rest of LLVM source code I would use typedef.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:158
+// Map from physical regunit to list of def SlotIndexes.
+using PhysDefsMap = DenseMap<unsigned, SmallVector<SlotIndex, 4>>;
+
----------------
qcolombet wrote:
> For consistency with the rest of LLVM source code I would use typedef.
Having read the code now, I believe it would be more efficient to map PhysReg to LI.
It would greatly simplify add/removeMappedPhysReg.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:221
   AU.addRequired<VirtRegMap>();
+  AU.addRequired<MachineBlockFrequencyInfo>();
+  AU.addPreserved<MachineBlockFrequencyInfo>();
----------------
We could gate that dependency on DisableRenameCopys.
That being said, if we move all that code in its own pass, that's not going to be a problem :).


================
Comment at: lib/CodeGen/VirtRegMap.cpp:296
+      bool FoundDef = false;
+      for (auto DI = DefIndexes.begin(); DI != DefIndexes.end(); ++DI)
+        if (*DI == DefIndex) {
----------------
Seems inefficient to use a vector here.
At the very least it should be sorted.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:336
+                                    const TargetRegisterInfo &TRI,
+                                    const VirtRegMap &VRM) {
+
----------------
Looks to me like this is reimplementing functionalities already available in the LiveRegMatrix class.
Reuse that instead.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:428
+    // Check the cost/benefit of renaming.
+    BlockFrequency DefFreq = MBFI->getBlockFreq(MI->getParent());
+    BlockFrequency Benefit(DefFreq);
----------------
Wasn't the goal to propagate useless copies, i.e., that should always be beneficial, right?

If you want to do more, then please add test cases covering that. Indeed, as far as I can tell, the changes in the test cases only cover the simple case of propagating useless copies.


================
Comment at: lib/CodeGen/VirtRegMap.cpp:491
+    // reg so kill flags computation will be correct.
+    assert(LI.getNumValNums() == 1);
+    SlotIndex SrcDefPrevSlot = LI.getValNumInfo(0)->def.getPrevSlot();
----------------
Add && "Msg" in the assert.

Also explain in the comment before the assert what would it take to extend that to multiple VNs.


https://reviews.llvm.org/D25347