[PATCH] D55301: RegAlloc: Allow targets to split register allocation

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.
Herald added a subscriber: jdoerfert.

Hi Matt,

Couple of nitpicks inline.
My online remaining concern is exposing ClearVirtRegs.

Cheers,
-Quentin



================
Comment at: lib/CodeGen/RegAllocBase.cpp:179
+  if (!ShouldAllocateClass(*TRI, RC))
+    return;
+
----------------
For debugging purposes, add a DEBUG statement for each case.


================
Comment at: lib/CodeGen/RegAllocFast.cpp:73
 
-    RegAllocFast() : MachineFunctionPass(ID), StackSlotForVirtReg(-1) {}
+    RegAllocFast(RegClassFilterFunc F = allocateAllRegClasses,
+                 bool ClearVirtRegs_ = true) :
----------------
Should this be `const RegClassFilterFunc &` everywhere?


================
Comment at: lib/CodeGen/RegAllocFast.cpp:78
+      StackSlotForVirtReg(-1),
+      ClearVirtRegs(ClearVirtRegs_) {
+    }
----------------
It feels dangerous to expose the ClearVirtRegs to me.
Could we deduce what has to be cleared based on what we allocate instead of exposing this?


================
Comment at: lib/CodeGen/RegAllocFast.cpp:1350
+  std::function<bool(const TargetRegisterInfo &TRI,
+                     const TargetRegisterClass &RC)> Ftor, bool ClearVirtRegs) {
+  return new RegAllocFast(Ftor, ClearVirtRegs);
----------------
Could we have just one createFastRegisterAllocator with default arguments?
(Also ClearVirtReg should disappear per my other comment IMO).


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:603
+
+}
+
----------------
Ditto: Just one createXXX method.


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

https://reviews.llvm.org/D55301