[SystemZ] Rework subreg structure to avoid TableGen warning

[uweigand]

Summary

We're currently seeing the following warning when building the SystemZ back-end:

warning: SubRegIndex SystemZ::subreg_h64 and SystemZ::subreg_h32 compose ambiguously as SystemZ::subreg_hh32 or SystemZ::subreg_h32

The problem appears to be that the VR128 class only has a high subreg defined (via subreg_h64), and that subreg itself also has a high subreg defined (via subreg_h32). TableGen then creates an automatic composition that allows directly accessing that composition as subreg_h32. However, the back-end also defined a *manual* composition of subreg_h64 and subreg_h32 that is defined as subreg_hh32., which is what causes the ambiguity.

The reason we need the explicit subreg_hh32 is that FP128 and GR128 both have both a low and a high 64-bit subreg (via subreg_l64 and subreg_h64), each of which itself has a high subreg via subreg_h32. Now the automatic TableGen composition here will allow accessing subreg_h32 (subreg_l64) implicitly as subreg_h32, and we need the explicit subreg_hh32 to access subreg_h32 (subreg_h64).

The reason why TableGen creates the implicit subreg_h32 (subreg_l64) but not subreg_h32 (subreg_h64) appears to be simply that in the list of subregs of FP128 and GR128, subreg_l64 is listed first.

This patch therefore removes the warning by simply reversing that order. This means that TableGen now automatically creates subreg_h32 as implicit composition for subreg_h32 (subreg_h64) on *all* register classes, and we create an explicit composition subreg_lh32 to access subreg_h32 (subreg_l64) on those registers that have a subreg_l64.

This causes no change in code generation, but avoids the warning.

[bjope]

lib/Target/SystemZ/SystemZRegisterInfo.td

64

We used to have subregs specified from high to low in our OOT target. But nowadays we follow how it seems to be done for all in-tree target and specify them from low to high instead. There are some problems that comes with having them specified from high to low. I think mainly related to debug information.

If you look at SystemZGenRegisterInfo.inc before your patch you get this table with offset/size information:

extern const MCRegisterInfo::SubRegCoveredBits SystemZSubRegIdxRanges[] = {
  { 65535, 65535 },
  { 32, 32 },	// subreg_h32
  { 64, 64 },	// subreg_h64
  { 96, 32 },	// subreg_hh32
  { 64, 32 },	// subreg_hl32
  { 0, 32 },	// subreg_l32
  { 0, 64 },	// subreg_l64

with this patch you instead get

extern const MCRegisterInfo::SubRegCoveredBits SystemZSubRegIdxRanges[] = {
  { 65535, 65535 },
  { 32, 32 },	// subreg_h32
  { 64, 64 },	// subreg_h64
  { 0, 32 },	// subreg_l32
  { 0, 64 },	// subreg_l64
  { 32, 32 },	// subreg_lh32
  { 0, 32 },	// subreg_ll32
};

So both subreg_h32 and subreg_lh32 is basically pointing out bit 32-63 in a register (counting from least significant bit). When doing the subreg composition tablegen actually seems to assume that the subregs are specified from low to high when calculating the offsets.

The SubRegIdxRanges table is for example used (in calls to getSubRegIdxOffset)
by StackMaps::parseOperand and DwarfExpression::addMachineReg when generating DWARF expressions mapping a variable to a register.

If I remember correctly swapping order of subregister indices like this will result in a different order when using MCSubRegIterator. In our case this gave in different results in the last part of DwarfExpression::addMachineReg ("Otherwise, attempt to find a covering set of sub-register numbers").

I got a feeling that you wanted subreg_h32 to also act as the old subreg_hh32. That will often work, but as described above, the debug info would probably get messed up in some situations.

So if you do not care about debug info, maybe this patch is good as-is. If you care about debug info you might need to add some more hacks to work around problems in (at least) DwarfExpression::addMachineReg and StackMaps::parseOperand.

Some alternatives that perhaps can be tried (to avoid the ambiguity warnings) may involve:

• Adding some dummy subreg index types to "fill the hole" when defining FPR64 and VR128 (they are currently lacking a low subreg).
• Avoid reusing subreg index types for different types of registers. I'm not familiar with the SystemZ register hierarchy, so maybe this isn't an option, but the idea is to for example use the current subreg_h32, subreg_l64 etc for GPR and introduce some new indicies such as fsubreg_h32 for the FPR registers.

Not sure if any of the above really helps.
I guess the last resort would be to accept the solution in D50977.

[uweigand]

Thanks for pointing out those debug info differences, I agree that this might be a problem.

As to using different subreg indices, that's what we used to have before @kparzysz 's patch here https://reviews.llvm.org/D50725 (which is the patch that introduced this warning). But as I understood that patch, it actually itself fixed a problem with subreg liveness tracking, see the commit message:

However, the ability to compose subreg_h64 with subreg_r32, and with
subreg_h32 and subreg_l32 at the same time makes the compositions be
treated as non-overlapping (leading to problems when tracking subreg
liveness). See D50468 for more details.

As to defining dummy low subregs, I'm also not sure that this is a good idea. These registers have the property that writing to the (singular) subreg may implicitly clobber the whole register, and I was given to understand that this fact can be modeled for TableGen by having the register *not* fully covered by subregs.

Trying a completely different track: the problem seems to be caused in the end by those implicitly defined subregs that TableGen comes up with. But at least for our target, those really only ever seem to cause problems and aren't really helpful at all. I'd much prefer to have no implicitly defined subregs at all, and explicitly define the four compositions I really need: subreg_ll32, subreg_lh32, subreg_hl32, and subreg_hh32. That would also make the usage of those subregs much clearer in back-end source code.

Could we not simply add a way for the target to tell TableGen to not define any implicit compositions?

[kparzysz]

In D54962#1311537, @uweigand wrote:

Could we not simply add a way for the target to tell TableGen to not define any implicit compositions?

TableGen uses subregister indices (including the synthesized ones) to create a set of new register classes, which it then uses to calculate weights/costs that are later used by the compiler in register pressure calculation and scheduling. Turning off these compositions will have some sort of a domino effect, and it's not immediately clear what the final result will be.

An alternative would be to stop TableGen from associating subregister indices of a given register with its super-registers, i.e. doing:
"if Reg is a subregister of Super, and Reg has a subregister index subreg_idx, then add subreg_idx to subregister indices of Super". Instead, this should be a composition that first gets Reg from Super, and then applies subreg_idx. This will come with its own set of consequences, which aren't clear at the moment either.

My only concern here is understanding the consequences of this kind of a solution. As long as there is nothing unintended or unexpected, it should be fine. I guess the only way to make progress is to try it and see what happens.

lib/Target/SystemZ/SystemZInstrFP.td

def LEDBRA : TernaryRRFe<"ledbra", 0xB344, FP32,  FP64>,
              Requires<[FeatureFPExtension]>;
 def LEXBRA : TernaryRRFe<"lexbra", 0xB346, FP128, FP128>,
              Requires<[FeatureFPExtension]>;
 def LDXBRA : TernaryRRFe<"ldxbra", 0xB345, FP128, FP128>,
              Requires<[FeatureFPExtension]>;
 
 let Predicates = [FeatureNoVectorEnhancements1] in {
   def : Pat<(f32 (fpround FP128:$src)),
-            (EXTRACT_SUBREG (LEXBR FP128:$src), subreg_hh32)>;
+            (EXTRACT_SUBREG (LEXBR FP128:$src), subreg_h32)>;
   def : Pat<(f64 (fpround FP128:$src)),
             (EXTRACT_SUBREG (LDXBR FP128:$src), subreg_h64)>;
 }
 
 // Extend register floating-point values to wider representations.
 def LDEBR : UnaryRRE<"ldebr", 0xB304, fpextend,  FP64,  FP32>;
 def LXEBR : UnaryRRE<"lxebr", 0xB306, null_frag, FP128, FP32>;
 def LXDBR : UnaryRRE<"lxdbr", 0xB305, null_frag, FP128, FP64>;

lib/Target/SystemZ/SystemZRegisterInfo.h

 namespace llvm {
 
 class LiveIntervals;
 
 namespace SystemZ {
 // Return the subreg to use for referring to the even and odd registers
 // in a GR128 pair.  Is32Bit says whether we want a GR32 or GR64.
 inline unsigned even128(bool Is32bit) {
-  return Is32bit ? subreg_hl32 : subreg_h64;
+  return Is32bit ? subreg_l32 : subreg_h64;
 }
 inline unsigned odd128(bool Is32bit) {
-  return Is32bit ? subreg_l32 : subreg_l64;
+  return Is32bit ? subreg_ll32 : subreg_l64;
 }
 } // end namespace SystemZ
 
 struct SystemZRegisterInfo : public SystemZGenRegisterInfo {
 public:
   SystemZRegisterInfo();
 
   /// getPointerRegClass - Return the register class to use to hold pointers.

lib/Target/SystemZ/SystemZRegisterInfo.cpp

 // somehow belongs in it. Otherwise, return GRX32.
 static const TargetRegisterClass *getRC32(MachineOperand &MO,
                                           const VirtRegMap *VRM,
                                           const MachineRegisterInfo *MRI) {
   const TargetRegisterClass *RC = MRI->getRegClass(MO.getReg());
 
   if (SystemZ::GR32BitRegClass.hasSubClassEq(RC) ||
       MO.getSubReg() == SystemZ::subreg_l32 ||
-      MO.getSubReg() == SystemZ::subreg_hl32)
+      MO.getSubReg() == SystemZ::subreg_ll32)
     return &SystemZ::GR32BitRegClass;
   if (SystemZ::GRH32BitRegClass.hasSubClassEq(RC) ||
       MO.getSubReg() == SystemZ::subreg_h32 ||
-      MO.getSubReg() == SystemZ::subreg_hh32)
+      MO.getSubReg() == SystemZ::subreg_lh32)
     return &SystemZ::GRH32BitRegClass;
 
   if (VRM && VRM->hasPhys(MO.getReg())) {
     unsigned PhysReg = VRM->getPhys(MO.getReg());
     if (SystemZ::GR32BitRegClass.contains(PhysReg))
       return &SystemZ::GR32BitRegClass;
     assert (SystemZ::GRH32BitRegClass.contains(PhysReg) &&
             "Phys reg not in GR32 or GRH32?");

lib/Target/SystemZ/SystemZRegisterInfo.td

 }
 
 class SystemZRegWithSubregs<string n, list<Register> subregs>
   : RegisterWithSubRegs<n, subregs> {
   let Namespace = "SystemZ";
 }
 
 let Namespace = "SystemZ" in {
-def subreg_l32   : SubRegIndex<32, 0>;  // Also acts as subreg_ll32.
-def subreg_h32   : SubRegIndex<32, 32>; // Also acts as subreg_lh32.
+def subreg_l32   : SubRegIndex<32, 0>;  // Also acts as subreg_hl32.
+def subreg_h32   : SubRegIndex<32, 32>; // Also acts as subreg_hh32.
 def subreg_l64   : SubRegIndex<64, 0>;
 def subreg_h64   : SubRegIndex<64, 64>;
-def subreg_hh32  : ComposedSubRegIndex<subreg_h64, subreg_h32>;
-def subreg_hl32  : ComposedSubRegIndex<subreg_h64, subreg_l32>;
+def subreg_lh32  : ComposedSubRegIndex<subreg_l64, subreg_h32>;
+def subreg_ll32  : ComposedSubRegIndex<subreg_l64, subreg_l32>;
 }
 
 // Define a register class that contains values of types TYPES and an
 // associated operand called NAME.  SIZE is the size and alignment
 // of the registers and REGLIST is the list of individual registers.
 multiclass SystemZRegClass<string name, list<ValueType> types, int size,
                            dag regList, bit allocatable = 1> {
   def AsmOperand : AsmOperandClass {

 
 // Lower 32 bits of one of the 16 64-bit general-purpose registers
 class GPR32<bits<16> num, string n> : SystemZReg<n> {
   let HWEncoding = num;
 }
 
 // One of the 16 64-bit general-purpose registers.
 class GPR64<bits<16> num, string n, GPR32 low, GPR32 high>
- : SystemZRegWithSubregs<n, [low, high]> {
+ : SystemZRegWithSubregs<n, [high, low]> {
   let HWEncoding = num;
-  let SubRegIndices = [subreg_l32, subreg_h32];
+  let SubRegIndices = [subreg_h32, subreg_l32];
   let CoveredBySubRegs = 1;
 }
 
 // 8 even-odd pairs of GPR64s.
 class GPR128<bits<16> num, string n, GPR64 low, GPR64 high>
- : SystemZRegWithSubregs<n, [low, high]> {
+ : SystemZRegWithSubregs<n, [high, low]> {
   let HWEncoding = num;
-  let SubRegIndices = [subreg_l64, subreg_h64];
+  let SubRegIndices = [subreg_h64, subreg_l64];
   let CoveredBySubRegs = 1;
 }
 
 // General-purpose registers
 foreach I = 0-15 in {
   def R#I#L : GPR32<I, "r"#I>;
   def R#I#H : GPR32<I, "r"#I>;
   def R#I#D : GPR64<I, "r"#I, !cast<GPR32>("R"#I#"L"), !cast<GPR32>("R"#I#"H")>,

 class FPR64<bits<16> num, string n, FPR32 high>
  : SystemZRegWithSubregs<n, [high]> {
   let HWEncoding = num;
   let SubRegIndices = [subreg_h32];
 }
 
 // 8 pairs of FPR64s, with a one-register gap inbetween.
 class FPR128<bits<16> num, string n, FPR64 low, FPR64 high>
- : SystemZRegWithSubregs<n, [low, high]> {
+ : SystemZRegWithSubregs<n, [high, low]> {
   let HWEncoding = num;
-  let SubRegIndices = [subreg_l64, subreg_h64];
+  let SubRegIndices = [subreg_h64, subreg_l64];
   let CoveredBySubRegs = 1;
 }
 
 // Floating-point registers.  Registers 16-31 require the vector facility.
 foreach I = 0-15 in {
   def F#I#S : FPR32<I, "f"#I>;
   def F#I#D : FPR64<I, "f"#I, !cast<FPR32>("F"#I#"S")>,
               DwarfRegNum<[!cast<DwarfMapping>("F"#I#"Dwarf").Id]>;

test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir

bb.8:
     %10 = LLILL 41639
     dead %10 = LOCGR %10, %4, 14, 6, implicit killed $cc
     CHIMux %92, 0, implicit-def $cc
     BRC 14, 6, %bb.5, implicit killed $cc
     J %bb.9
   
   bb.9:
     %82 = VLVGP %67.subreg_h64, %67.subreg_h64
-    %82 = VLVGH %82, %58.subreg_hl32, $noreg, 0
-    %82 = VLVGH %82, %80.subreg_hl32, $noreg, 1
-    dead %82 = VLVGH %82, %90.subreg_hl32, $noreg, 2
+    %82 = VLVGH %82, %58.subreg_l32, $noreg, 0
+    %82 = VLVGH %82, %80.subreg_l32, $noreg, 1
+    dead %82 = VLVGH %82, %90.subreg_l32, $noreg, 2
     %96 = AFIMux %96, 1879048192, implicit-def dead $cc
     %96 = SRL %96, $noreg, 31
     dead %11 = VLVGF %11, %96, $noreg, 1
     %100 = LHIMux 0
   
   bb.10:
     CHIMux %100, 0, implicit-def $cc
     BRC 14, 6, %bb.10, implicit killed $cc
     J %bb.11
   
   bb.11:
     Return
 
 ...

test/CodeGen/SystemZ/cond-move-05.mir

registers:
   - { id: 9, class: gr128bit }
   - { id: 10, class: gr64bit }
 body:             |
   bb.0.entry:
     %0:gr64bit = LGFRL @g_74 :: (dereferenceable load 4 from @g_74)
     undef %3.subreg_l64:gr128bit = LGHI 1
     %3.subreg_h64:gr128bit = LLILL 0
     %3:gr128bit = DLGR %3, %0
-    CLFIMux %3.subreg_hl32, 3631842929, implicit-def $cc
-    %6:grx32bit = LOCRMux undef %6, %3.subreg_hl32, 14, 4, implicit killed $cc
+    CLFIMux %3.subreg_l32, 3631842929, implicit-def $cc
+    %6:grx32bit = LOCRMux undef %6, %3.subreg_l32, 14, 4, implicit killed $cc
     CHIMux %6, 0, implicit-def $cc
     BRC 14, 8, %bb.2.for.inc591.1.i.i, implicit killed $cc
     J %bb.1.cleanup584.i.i
   
   bb.1.cleanup584.i.i:
     successors: 
   
   
   bb.2.for.inc591.1.i.i:
 
 ...

test/CodeGen/SystemZ/regalloc-GR128-02.mir

bb.0 (%ir-block.0):
     J %bb.1
   
   bb.1 (%ir-block.4):
     %8:gr32bit = LHIMux -9
     STRL killed %8, @g_74 :: (store 4 into @g_74)
     Return
   
   bb.2 (%ir-block.5):
-    dead %0:grx32bit = COPY undef %6.subreg_l32
+    dead %0:grx32bit = COPY undef %6.subreg_ll32
 
 ...

test/CodeGen/SystemZ/regalloc-fast-invalid-kill-flag.mir

 # Although R0L partially redefines R0Q, it must not mark R0Q as kill
 # because R1D is still live through that instruction.
 # CHECK-NOT: implicit killed $r0q
 # CHECK-NEXT: {{\$r[0-9]+d}} = COPY renamable $r1d
 # CHECK-NEXT: LARL
 body:             |
   bb.0:
     %0 : gr128bit = IMPLICIT_DEF
-    %0.subreg_hl32 = COPY %0.subreg_l32
+    %0.subreg_l32 = COPY %0.subreg_ll32
     %1 : gr64bit = COPY %0.subreg_l64
     %2 : addr64bit = LARL @g_167
     STC %1.subreg_l32, %2, 8, $noreg
 
 ...