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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

> I'm not sure I follow this. These aren't spilled with ordinary copies

I would expect that you could use ordinary copies + subreg here and do the proper expansion in the later expand post RA pass like every other copy.

> This uses cross lane instructions to read/write SGPRs into the various lane VGPRs (i.e 64 SGPRs can be spilled to each lane in the wave's VGPR). We also can't legally copy from V to S. Having virtual registers with the combined class doesn't really conceptually make sense for us either (and would probably break every single place that we need to consider these)

That wouldn't appear in elsewhere than tablegen. That's just something to tell RA that the biggest unconstrained class is V+S.

> This also wouldn't allow us to change the set of reserved registers in the middle of allocation, which is part of the problem.

I missed that part, but I also don't get why this is a problem. IIRC we can always narrow the set of available registers for each virtual register.

Anyhow, the changes on the generic parts looks mostly good  to me. Comments inlined.



================
Comment at: include/llvm/CodeGen/RegAllocCommon.h:23
+
+static inline bool allocateAllRegClasses(const TargetRegisterInfo &,
+                                         const TargetRegisterClass &) {
----------------
Please add doxygen comment.


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:615
+  RegAllocBase() {
+
+}
----------------
Why do we need both constructors?


================
Comment at: lib/CodeGen/RegAllocGreedy.cpp:707
   const unsigned Reg = LI->reg;
-  assert(TargetRegisterInfo::isVirtualRegister(Reg) &&
-         "Can only enqueue virtual registers");
   unsigned Prio;
 
----------------
Removing this assert is worrisome. Why do we need that?


================
Comment at: lib/CodeGen/TargetFrameLoweringImpl.cpp:21
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
+#include "llvm/CodeGen/VirtRegMap.h"
 #include "llvm/IR/Attributes.h"
----------------
Why do we need this change?


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

https://reviews.llvm.org/D55301