[InstrEmitter, SystemZ] Copy Access registers with the correct register class.

[jonpa]

Summary

On SystemZ there are a set of "access registers" that can be copied in and out of 32-bit GPRs with special instructions. These instructions can only perform the copy using low 32-bit parts of the 64-bit GPRs. As reported and discussed at https://bugs.llvm.org/show_bug.cgi?id=44254, this is currently broken due to the fact that the default register class for 32-bit integers is GRX32, which also contains the high 32-bit part registers.

I have tried to find a simple way to constrain the register class of such copies (also at -O0), but this turned out to not be quite simple. Just selecting a pseudo instruction with a custom inserter does not seem to work since CopyToReg/CopyFromReg have special handlings in InstrEmitter.

I then tried in SystemZDAGToDAG.cpp to select a CopyToReg to (CopyToReg COPY_TO_REGCLASS), which worked fine. But I could not get the same results with CopyFromReg. (COPY_TO_REGCLASS CopyFromReg) only resulted in a later COPY into GR32, but the COPY from the Access register was still first made to GRX32.

One alternative might be to let InstrEmitter deduce the needed register class for a CopyFromReg if there is only one user which is a COPY_TO_REGCLASS. I thought this seemed a bit messy, so I instead tried adding a new TargetRegisterInfo hook that is used in InstrEmitter when emitting CopyToReg/CopyFromReg nodes.

Does this make sense, or is there a better way?

[jonpa]

This is an "uglier" handling that does not involve any common code changes. The reason it is not so nice is that the backend needs to transform the SelectionDAG (in Select()) by first recognizing any copy to/from a special set of registers and then in the case of copyFromReg insert a target pseudo opcode just with the purpose of constraining the regclass of the created virtreg.

It would be much simpler if the target could just supply the right regclass in the first place, which was my original suggestion (please see "Diff 1" under the History tab).

[hliao]

Removed from reviewer list as the new patch takes all target-specific approach.

[jonpa]

In D75014#1890295, @hliao wrote:

Removed from reviewer list as the new patch takes all target-specific approach.

Sorry - I meant to post these two approaches side-by-side, and personally I hope that the target-specific approach will not end up being used... So any comments on this would be much appreciated still! Do you think the new TRI seems reasonable?

[uweigand]

I'm wondering if this handles all cases ... for CopyFromReg you apparently rely on the logic in EmitCopyFromReg that checks whether the value is used by some MachineNode with constrained regclass. But that logic isn't unconditionally used, e.g. it is skipped for "cloned" SUs ... not sure whether this could cause issues in more complicated scenarios.

Also, I'm not sure if the Glue handling is fully correct: for CopyToReg, you seem to simply drop the glue, which doesn't look right. For CopyFromReg, you keep the glue ... but you also glue the new node in as well, which may not be necessary (and may actually confuse code in EmitNode that scans glue chains?).

That said, if we do need to handle this problem in target code, it might actually be cleaner to just do it as a separate PreReload pass that simply looks at COPY nodes and constrains source/target virtual registers if required. This could be something like the X86 FlagsCopyLowering pass, I guess (but much simpler).

(Talking about flags, it seems that COPY from/to the %cc register would also require the same handling as access registers. Well, I guess we could implement a copy *to* %cc from a high register using TMHH, but we cannot implement a copy from %cc to a high register ...)

[uweigand]

Oh, and one more thing: either way, can you please add the original test case from D74601 so we're sure this problem is (and remains) fixed. Thanks!

[jonpa]

In D75014#1891349, @uweigand wrote:

Oh, and one more thing: either way, can you please add the original test case from D74601 so we're sure this problem is (and remains) fixed. Thanks!

I added the two test functions I could find already as @_ZTW1x -> tls-08.ll:@fun0, and @_Z6squareiiiiiii -> tls-09.ll.

[uweigand]

In D75014#1891587, @jonpa wrote:

In D75014#1891349, @uweigand wrote:

Oh, and one more thing: either way, can you please add the original test case from D74601 so we're sure this problem is (and remains) fixed. Thanks!

I added the two test functions I could find already as @_ZTW1x -> tls-08.ll:@fun0, and @_Z6squareiiiiiii -> tls-09.ll.

Ah, I missed that, sorry. That's fine then.

[jonpa]

... That said, if we do need to handle this problem in target code, it might actually be cleaner to just do it as a separate PreReload pass that simply looks at COPY nodes and constrains source/target virtual registers if required. This could be something like the X86 FlagsCopyLowering pass, I guess (but much simpler).

Handling this with a new pre-regalloc pass instead that transforms COPY instructions of special physregs before register allocation. An alternative to this would be to do this in EmitInstrWithCustomInserter(), but currently COPY instructions are not handled there.

• It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.
• I don't think kill flags are useful to manage here, so they are ignored.

(Talking about flags, it seems that COPY from/to the %cc register would also require the same handling as access registers. Well, I guess we could implement a copy *to* %cc from a high register using TMHH, but we cannot implement a copy from %cc to a high register ...)

Handling also COPYs of CC. Copy *to* CC is now done either with TMLH or TMHH (depending on the source reg) in copyPhysReg(). A copy from CC is handled in this new pass instead.

[uweigand]

It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.

I'm not sure I understand this: can you explain what problem you see with constraining the register class?

Also, if you do directly emit the EAR etc., I'm wondering why you still keep the COPY in as well?

[jonpa]

In D75014#1897768, @uweigand wrote:

It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.

I'm not sure I understand this: can you explain what problem you see with constraining the register class?

I remember seeing that the register allocator would create a new virtual register and give it the register class based on calling MI->getRegClassConstraint() (or TII->getRegClass() directly). So in theory, it seems that if there is no MCInstrDesc anywhere that demands a particular register class, regalloc might feel free to take the optimal one (GRX32). I am not sure this is needed, but there is no mechanism that I know of that would constrain a *COPY* register regclass, although it may be that a COPY of a physreg into a virtreg is left alone.

Maybe someone could instead confirm that physreg copies do not get their virtreg regclasses changed ever. Maybe that is obvious and I just wasn't aware. Or, if there is no guarantee for this, perhaps a target hook like I suggested (getPhysRegCopyRegClass()) would be a better solution after all, since that would also then be an error if regalloc broke that.

Also, if you do directly emit the EAR etc., I'm wondering why you still keep the COPY in as well?

I figured that the coalescer should typically remove it. And I wasn't sure if there could ever be a problem with any other connected virtregs involved having to use a high-register. In that case there would have to be a copy to/from a GRH32.

[uweigand]

In D75014#1898783, @jonpa wrote:

In D75014#1897768, @uweigand wrote:

It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.

I'm not sure I understand this: can you explain what problem you see with constraining the register class?

I remember seeing that the register allocator would create a new virtual register and give it the register class based on calling MI->getRegClassConstraint() (or TII->getRegClass() directly). So in theory, it seems that if there is no MCInstrDesc anywhere that demands a particular register class, regalloc might feel free to take the optimal one (GRX32). I am not sure this is needed, but there is no mechanism that I know of that would constrain a *COPY* register regclass, although it may be that a COPY of a physreg into a virtreg is left alone.

Maybe someone could instead confirm that physreg copies do not get their virtreg regclasses changed ever. Maybe that is obvious and I just wasn't aware. Or, if there is no guarantee for this, perhaps a target hook like I suggested (getPhysRegCopyRegClass()) would be a better solution after all, since that would also then be an error if regalloc broke that.

Hmm, I see. I would expect that the one virtreg that was constrained will certainly keep its register class. However, I guess you're right that regalloc may create *new* virtregs (e.g. due to live range splitting etc.). I'm not sure either how the case where the virtreg is a COPY of a phyregs will be handled in that case. Thanks for pointing this out.

Also, if you do directly emit the EAR etc., I'm wondering why you still keep the COPY in as well?

I figured that the coalescer should typically remove it. And I wasn't sure if there could ever be a problem with any other connected virtregs involved having to use a high-register. In that case there would have to be a copy to/from a GRH32.

OK. Well, it's certainly conservatively correct to do it this way.

Given that the patch as-is looks correct to me, and it does fix an actual bug, I think we should go ahead with it for now. If we find another solution later (e.g. via common code change), we can always take the SystemZ pass out again. LGTM.

llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp

824 ↗

(On Diff #246851)

I'm wondering if we need to check (or at least assert) that SrcReg is even a GRX32 here?

[jonpa]

llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp

824 ↗

(On Diff #246851)

The MachineVerifier would catch this with "Illegal physical register for instruction". There are no subregs anymore at this point (after regalloc), so it would be a very strange error. Perhaps an assert that copyPhysReg() is only called with two physreg operands (post-RA) would make sense?

It seems that without the verifier a COPY from $r4d in tls-11.mir does not get caught anywhere.

llvm/include/llvm/CodeGen/TargetRegisterInfo.h

public:
   /// to or from. If it is possible to copy the register directly without using
   /// a cross register class copy, return the specified RC. Returns NULL if it
   /// is not possible to copy between two registers of the specified class.
   virtual const TargetRegisterClass *
   getCrossCopyRegClass(const TargetRegisterClass *RC) const {
     return RC;
   }
 
+  /// Returns the needed register class for a virtual register involved in
+  /// copying a value to/from PhysReg. If nullptr is returned the default
+  /// register class based on the type is used.
+  virtual const TargetRegisterClass *
+  getPhysRegCopyRegClass(unsigned PhysReg) const {
+    return nullptr;
+  }
+
   /// Returns the largest super class of RC that is legal to use in the current
   /// sub-target and has the same spill size.
   /// The returned register class can be used to create virtual registers which
   /// means that all its registers can be copied and spilled.
   virtual const TargetRegisterClass *
   getLargestLegalSuperClass(const TargetRegisterClass *RC,
                             const MachineFunction &) const {
     /// The default implementation is very conservative and doesn't allow the

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

if (VRBase) {
     DstRC = MRI->getRegClass(VRBase);
   } else if (UseRC) {
     assert(TRI->isTypeLegalForClass(*UseRC, VT) &&
            "Incompatible phys register def and uses!");
     DstRC = UseRC;
   } else {
     DstRC = TLI->getRegClassFor(VT, Node->isDivergent());
   }
+  if (auto *CopyRC = TRI->getPhysRegCopyRegClass(SrcReg))
+    DstRC = CopyRC;
 
   // If all uses are reading from the src physical register and copying the
   // register is either impossible or very expensive, then don't create a copy.
   if (MatchReg && SrcRC->getCopyCost() < 0) {
     VRBase = SrcReg;
   } else {
     // Create the reg, emit the copy.
     VRBase = MRI->createVirtualRegister(DstRC);
case ISD::CopyToReg: {
     if (RegisterSDNode *R = dyn_cast<RegisterSDNode>(SrcVal))
       SrcReg = R->getReg();
     else
       SrcReg = getVR(SrcVal, VRBaseMap);
 
     if (SrcReg == DestReg) // Coalesced away the copy? Ignore.
       break;
 
+    if (auto *CopyRC = TRI->getPhysRegCopyRegClass(DestReg)) {
+      unsigned TmpReg = MRI->createVirtualRegister(CopyRC);
+      BuildMI(*MBB, InsertPos, Node->getDebugLoc(), TII->get(TargetOpcode::COPY),
+            TmpReg).addReg(SrcReg);
+      SrcReg = TmpReg;
+    }
+
     BuildMI(*MBB, InsertPos, Node->getDebugLoc(), TII->get(TargetOpcode::COPY),
             DestReg).addReg(SrcReg);
     break;
   }
   case ISD::CopyFromReg: {
     unsigned SrcReg = cast<RegisterSDNode>(Node->getOperand(1))->getReg();
     EmitCopyFromReg(Node, 0, IsClone, IsCloned, SrcReg, VRBaseMap);
     break;

llvm/lib/Target/SystemZ/SystemZRegisterInfo.h

public:
   }
 
   /// getCrossCopyRegClass - Returns a legal register class to copy a register
   /// in the specified class to or from. Returns NULL if it is possible to copy
   /// between a two registers of the specified class.
   const TargetRegisterClass *
   getCrossCopyRegClass(const TargetRegisterClass *RC) const override;
 
+  const TargetRegisterClass *
+  getPhysRegCopyRegClass(unsigned PhysReg) const override;
+
   bool getRegAllocationHints(unsigned VirtReg,
                              ArrayRef<MCPhysReg> Order,
                              SmallVectorImpl<MCPhysReg> &Hints,
                              const MachineFunction &MF,
                              const VirtRegMap *VRM,
                              const LiveRegMatrix *Matrix) const override;
 
   // Override TargetRegisterInfo.h.

llvm/lib/Target/SystemZ/SystemZRegisterInfo.cpp

 
 const TargetRegisterClass *
 SystemZRegisterInfo::getCrossCopyRegClass(const TargetRegisterClass *RC) const {
   if (RC == &SystemZ::CCRRegClass)
     return &SystemZ::GR32BitRegClass;
   return RC;
 }
 
+const TargetRegisterClass *
+SystemZRegisterInfo::getPhysRegCopyRegClass(unsigned PhysReg) const {
+  // EAR / SAR can only use GR32 registers.
+  if (SystemZ::AR32BitRegClass.contains(PhysReg))
+    return &SystemZ::GR32BitRegClass;
+  return nullptr;
+}

llvm/test/CodeGen/SystemZ/tls-08.ll

+; RUN: llc < %s -mcpu=z196 -mtriple=s390x-linux-gnu -O0 -stop-after=finalize-isel \
+; RUN:   2>&1 | FileCheck %s
+; REQUIRES: asserts
+;
+; Test that copies to/from access registers are emitted from isel with GR32 regs.
+
+@x = dso_local thread_local global i32 0, align 4
+define weak_odr hidden i32* @fun0() {
+; CHECK: name:            fun0
+; CHECK: %{{[0-9]+}}:gr32bit = COPY $a0
+; CHECK: %{{[0-9]+}}:gr32bit = COPY $a1
+  ret i32* @x
+}
+
+define i32 @fun1() {
+; CHECK: name:            fun1
+; CHECK: %1:gr32bit = COPY %0
+; CHECK: $a1 = COPY %1
+; CHECK: %2:gr32bit = COPY $a0
+  %val = call i32 asm "blah", "={a0}, {a1}" (i32 0)
+  ret i32 %val
+}

llvm/test/CodeGen/SystemZ/tls-09.ll

+; RUN: llc < %s -mcpu=z196 -mtriple=s390x-linux-gnu -O0
+;
+; Test that a0 and a1 are copied successfully into GR32 registers.
+
+@x = dso_local thread_local global i32 0, align 4
+define i32 @fun0(i32 signext, i32 signext, i32 signext, i32 signext, i32 signext, i32 signext, i32 signext)  {
+  %8 = alloca i32, align 4
+  %9 = alloca i32, align 4
+  %10 = alloca i32, align 4
+  %11 = alloca i32, align 4
+  %12 = alloca i32, align 4
+  %13 = alloca i32, align 4
+  %14 = alloca i32, align 4
+  %15 = load i32, i32* @x, align 4
+  store i32 %0, i32* %8, align 4
+  store i32 %1, i32* %9, align 4
+  store i32 %2, i32* %10, align 4
+  store i32 %3, i32* %11, align 4
+  store i32 %4, i32* %12, align 4
+  store i32 %5, i32* %13, align 4
+  store i32 %6, i32* %14, align 4
+  %16 = load i32, i32* %8, align 4
+  %17 = add nsw i32 %15, %16
+  %18 = load i32, i32* %9, align 4
+  %19 = add nsw i32 %17, %18
+  %20 = load i32, i32* %10, align 4
+  %21 = add nsw i32 %19, %20
+  %22 = load i32, i32* %11, align 4
+  %23 = add nsw i32 %21, %22
+  %24 = load i32, i32* %12, align 4
+  %25 = add nsw i32 %23, %24
+  %26 = load i32, i32* %13, align 4
+  %27 = add nsw i32 %25, %26
+  %28 = load i32, i32* %14, align 4
+  %29 = add nsw i32 %27, %28
+  ret i32 %29
+}