[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Wei,

Couple of comments:

#1

- Out of curiosity, do you have numbers of how many redundant spills we have for the current solution?
- I am not saying the live-range splitting mechanism is perfect, but it fits nicely to the exiting framework, in particular w.r.t. the way we model the spill cost. Which leads me to #2.

#2

- hoistCopiesForSize is for Copies AFAIR, i.e., split points not spills. That means that, IIRC, we have the save cost model, since we do not care about split insertion in the cost.

#3

- I agree the spill hoisting thing is more like a post reg alloc phase. Therefore, I would rather have it a post regalloc pass instead of embedded in the spiller. That being said, I understand this is easier to directly work with the virtual registers.

To summarize, this is fine to have a the spill hoisting optimization where you put it. I believe though that making the splitting smarter would be the first logical step to mitigate the need for such optimization and I am still concerned that it may be bad for compile time.

If you’d like to pursue into that direction anyway, it is okay, just couple of inlined comments.

As for path part 2, it does not do anything at the moment does? (I.e., we clear the set before walking through it).

Thanks,
-Quentin


================
Comment at: include/llvm/CodeGen/VirtRegMap.h:66
@@ +65,3 @@
+    /// its siblings mapping.
+    DenseMap<unsigned, DenseSet<unsigned>> Virt2SiblingsMap;
+
----------------
Is there a way this could be computed from the split map?

================
Comment at: lib/CodeGen/InlineSpiller.cpp:161
@@ -153,1 +160,3 @@
 
+  void addToMergableSpills(MachineInstr *Spill);
+  void rmFromMergableSpills(MachineInstr *Spill);
----------------
Can set private, right?

================
Comment at: lib/CodeGen/InlineSpiller.cpp:162
@@ +161,3 @@
+  void addToMergableSpills(MachineInstr *Spill);
+  void rmFromMergableSpills(MachineInstr *Spill);
+  bool isSpillCandBB(unsigned OrigReg, VNInfo *OrigVNI, MachineBasicBlock *BB,
----------------
Ditto.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:164
@@ +163,3 @@
+  bool isSpillCandBB(unsigned OrigReg, VNInfo *OrigVNI, MachineBasicBlock *BB,
+                     unsigned &LiveReg);
+  void runHoistSpills(unsigned OrigReg, VNInfo *OrigVNI,
----------------
Ditto.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:168
@@ +167,3 @@
+                      SmallPtrSet<MachineInstr *, 16> &SpillsToRm,
+                      DenseMap<MachineBasicBlock *, unsigned> &SpillsToIns);
+  void hoistAllSpills() override;
----------------
Ditto.

================
Comment at: lib/CodeGen/Spiller.h:34
@@ -32,1 +33,3 @@
+    /// and remove redundent spills.
+    virtual void hoistAllSpills() = 0;
   };
----------------
Call this method postOptimization and make it a non-abstract method.
We do not want the spillers existing out of tree to have to add a default implementation whereas they do not need to do anything.


Repository:
  rL LLVM

http://reviews.llvm.org/D15302