[PATCH] D37899: [SystemZ] Implement shouldCoalesce() to help regalloc to avoid running out of registers with GR128 regs

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZRegisterInfo.cpp:166
+  // Coalesce anything which is not a COPY involving a subreg to/from GR128.
+  if (!(NewRC->hasSubClassEq(&SystemZ::GR128BitRegClass) &&
+        (getRegSizeInBits(*SrcRC) <= 64 || getRegSizeInBits(*DstRC) <= 64)))
----------------
uweigand wrote:
> Hmm, does this still hit if we get ADDR128BitRegClass?   Probably needs to check for that instead of GR128BitRegClass here ...
> 
> We then ideally should also have a test case that uses ADDR128BitRegClass (one way to achieve this is to add a use of "remainder + 1", which gets selected to an LA instruction).
It should be NewRC->hasSuperClassEq(), which I believe returns true only for ADDR128 and GR128. I verified that this works as expected in gdb.




================
Comment at: lib/Target/SystemZ/SystemZRegisterInfo.cpp:199
+  // pairs in the region.
+  unsigned MICount = 0;
+  BitVector PhysClobbered(getNumRegs());
----------------
uweigand wrote:
> Unused now?
removed


================
Comment at: lib/Target/SystemZ/SystemZRegisterInfo.cpp:209
+        else if (SystemZ::GRX32BitRegClass.contains(MO.getReg()) ||
+                 SystemZ::GR64BitRegClass.contains(MO.getReg())) {
+          for (MCSuperRegIterator SI(MO.getReg(), this, false/*IncludeSelf*/);
----------------
uweigand wrote:
> Is this really necessary / useful?   Why not do the whole test just via the SuperRegIterator loop (using IncludeSelf = true)?
removed


https://reviews.llvm.org/D37899