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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In https://reviews.llvm.org/D37899#881218, @jonpa wrote:

> Considering other types of code with more heavy use of GR128, would we perhaps want to also give a count for a virtual register definition of a GR128? That should avoid being too optimistic when several of these overlap.


Not sure, hard to say if this is going to be overall beneficial or not.  I'd say to just do the physreg check for now.

> Great majority of these cases are then as expected small regions, so I am thinking there's no real need for an instruction limit - at least not on SPEC

OK, makes sense to me.

The code now looks reasonable to me, just a few 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)))
----------------
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).


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


================
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*/);
----------------
Is this really necessary / useful?   Why not do the whole test just via the SuperRegIterator loop (using IncludeSelf = true)?


https://reviews.llvm.org/D37899