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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Wei,

Could you rebase your patches, they do not apply cleanly for me.

Also, while I am here, a couple of inlined comments :).

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:148
@@ -143,2 +147,3 @@
   unsigned get(unsigned idx) const { return NewRegs[idx+FirstNew]; }
+  void pop_back() { NewRegs.pop_back(); }
 
----------------
This seems strange that the API allows to drop some of the new registers.
At the very least, we should document (i.e., put explanatory comments) why this is useful and why it is okay to drop such references.
In general, it should not.

================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:184
@@ +183,3 @@
+    VNInfo *OrigVNI;        // ParentVNI.def may be a copy only. OrigVNI.def
+                            // contains the real expr for remat.
+    MachineInstr *OrigMI;   // Instruction defining OrigVNI.
----------------
Instead of having at additional field which may be the same as ParentVNI in a lot of cases, this one could be computed.
Then, if this has some performance problem, we can think of a better caching mechanism.

================
Comment at: include/llvm/CodeGen/LiveRangeEdit.h:235
@@ -220,3 +234,3 @@
   /// as currently those new intervals are not guaranteed to spill.
-  void eliminateDeadDefs(SmallVectorImpl<MachineInstr*> &Dead,
-                         ArrayRef<unsigned> RegsBeingSpilled = None);
+  /// NoSplit indicates it is used after the iterations of selectOrSplit and
+  /// registers should not be split into new intervals.
----------------
Replace 'it' by this live interval or something.
The context is now high in the source file and repeating it wouldn't hurt IMO.

================
Comment at: lib/CodeGen/InlineSpiller.cpp:56
@@ -57,1 +55,3 @@
 namespace {
+class HoistSpiller {
+  MachineFunction &MF;
----------------
Since this class does not inherit from Spiller, what about naming it HoistSpillHelper or something.


Repository:
  rL LLVM

http://reviews.llvm.org/D15302