[SelectionDAG] Provide adequate register class for RegisterSDNode

[smaksimovic]

Summary

When adding operands to machine instructions in case of RegisterSDNodes,
generate a COPY node in case the register class does not match the one in the instruction definition.

[smaksimovic]

Ping

[sdardis]

I think the approach here is incorrect, as this patch is solving a very specific case when the bug is more general.

During the instruction emission of SDNodes to MachineInstrs, InstrEmitter::AddOperand() fails to account
for handling register operands whose register class differs from the instruction definition.

Normally the initial register class for an incoming register operand is chosen based on the value type it
holds, which in this case is GPR32. The problematic SW16_MM however takes a GPRMM16Zero register
operand.

Can you look at modifying MachineInstrs, InstrEmitter::AddOperand() to use AddRegisterOperand in the
case of RegisterSDNodes?

Thanks,
Simon

[smaksimovic]

Compare the register classes one would get for the machine instruction operand Op as opposed to the register class we would get based on the MCInstrDesc.
If those two would differ, we would emit an additional COPY node before adding the register which now has the desired register class.
Tried using the AddRegisterOperand() directly, but couldn't do so since getVR called inside cannot look up the SDValue Op inside the map -- that value happens to be a COPY node as well for which .isMachineOpcode() returns false.
As far as I can tell, additions to that map are being done in functions called from InstrEmitter::EmitMachineNode() which I assume deals with machine nodes only.

[smaksimovic]

Ping

[smaksimovic]

Ping

[sdardis]

I believe the entire hunk for handling RegisterSDNodes should be sunk into AddRegisterOperand as your change mimics portions of the logic there.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

423

Space after the if, and the line is overly long.

[smaksimovic]

Ping

[sdardis]

Please update the title with [SelectionDAG] tag and description to reflect that this is now a selection dag patch. Also, can you restore the code to where it previously was, I was incorrect about that, as it'd just make the code harder to follow in terms of handling virtual and physical registers in the same function.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

319–321

This comment should go above:

bool Imp = II && (IIOpNum >= II->getNumOperands() && !II->isVariadic());

330

Formatting, space after the if.

[sdardis]

It appears this also fixes a machine verifier failure in test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll, but I am unsure on the correctness of the changed assembly output. +CC'ing Jan Vesely who provided the test.

[jvesely]

In D35561#996084, @sdardis wrote:

It appears this also fixes a machine verifier failure in test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll, but I am unsure on the correctness of the changed assembly output. +CC'ing Jan Vesely who provided the test.

It'd help to see the diff. The test checks that certain registers are reserved and not allocated when thread IDs are used by the shader.
TIDIG.{x,y,z} is in T0.{x,y,z}, TGID.{x,y,z} is in T1.{x,y,z}.

It looks like this patch is OK other than introducing an extra MOV, I haven't looked into why that is.

[sdardis]

Attached.

AMDGPU-D35561.diff2 KBDownload

[jvesely]

In D35561#999000, @sdardis wrote:

Attached.

AMDGPU-D35561.diff2 KBDownload

These look good. An additional MOV is inserted since MEMRAT_CACHELESS STORE_RAW includes constraint of using _X subreg for 32bit stores.
When you edit the test please make sure to test both the MOV from the correct id register and store of the right data.

thank you.

[smaksimovic]

Update test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

[jvesely]

In D35561#1000382, @smaksimovic wrote:

Update test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

the r600 test LGTM

[sdardis]

LGTM.

test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

1

Add -verify-machineinstrs here to ensure we don't regress.

[smaksimovic]

rL324733

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

 void
 InstrEmitter::AddRegisterOperand(MachineInstrBuilder &MIB,
                                  SDValue Op,
                                  unsigned IIOpNum,
                                  const MCInstrDesc *II,
                                  DenseMap<SDValue, unsigned> &VRBaseMap,
                                  bool IsDebug, bool IsClone, bool IsCloned) {
   assert(Op.getValueType() != MVT::Other &&
          Op.getValueType() != MVT::Glue &&
          "Chain and glue operands should occur at end of operand list!");
   // Get/emit the operand.
   unsigned VReg = getVR(Op, VRBaseMap);
 
   const MCInstrDesc &MCID = MIB->getDesc();
   bool isOptDef = IIOpNum < MCID.getNumOperands() &&
     MCID.OpInfo[IIOpNum].isOptionalDef();
 
   // If the instruction requires a register in a different class, create
   // a new virtual register and copy the value into it, but first attempt to
   // shrink VReg's register class within reason.  For example, if VReg == GR32
   // and II requires a GR32_NOSP, just constrain VReg to GR32_NOSP.
   if (II) {
     const TargetRegisterClass *OpRC = nullptr;
     if (IIOpNum < II->getNumOperands())
       OpRC = TII->getRegClass(*II, IIOpNum, TRI, *MF);
 
     if (OpRC) {
       const TargetRegisterClass *ConstrainedRC
void InstrEmitter::AddOperand(MachineInstrBuilder &MIB,
   if (Op.isMachineOpcode()) {
     AddRegisterOperand(MIB, Op, IIOpNum, II, VRBaseMap,
                        IsDebug, IsClone, IsCloned);
   } else if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Op)) {
     MIB.addImm(C->getSExtValue());
   } else if (ConstantFPSDNode *F = dyn_cast<ConstantFPSDNode>(Op)) {
     MIB.addFPImm(F->getConstantFPValue());
   } else if (RegisterSDNode *R = dyn_cast<RegisterSDNode>(Op)) {
+    unsigned VReg = R->getReg();
+    MVT OpVT = Op.getSimpleValueType();
+    const TargetRegisterClass *OpRC =
+        TLI->isTypeLegal(OpVT) ? TLI->getRegClassFor(OpVT) : nullptr;
+    const TargetRegisterClass *IIRC =
+        II ? TRI->getAllocatableClass(TII->getRegClass(*II, IIOpNum, TRI, *MF))
+           : nullptr;
+
+    if (OpRC && IIRC && OpRC != IIRC &&
+        TargetRegisterInfo::isVirtualRegister(VReg)) {
+      unsigned NewVReg = MRI->createVirtualRegister(IIRC);
+      BuildMI(*MBB, InsertPos, Op.getNode()->getDebugLoc(),
+               TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg);
+      VReg = NewVReg;
+    }
     // Turn additional physreg operands into implicit uses on non-variadic
     // instructions. This is used by call and return instructions passing
     // arguments in registers.
     bool Imp = II && (IIOpNum >= II->getNumOperands() && !II->isVariadic());
-    MIB.addReg(R->getReg(), getImplRegState(Imp));
+    MIB.addReg(VReg, getImplRegState(Imp));
   } else if (RegisterMaskSDNode *RM = dyn_cast<RegisterMaskSDNode>(Op)) {
     MIB.addRegMask(RM->getRegMask());
   } else if (GlobalAddressSDNode *TGA = dyn_cast<GlobalAddressSDNode>(Op)) {
     MIB.addGlobalAddress(TGA->getGlobal(), TGA->getOffset(),
                          TGA->getTargetFlags());
   } else if (BasicBlockSDNode *BBNode = dyn_cast<BasicBlockSDNode>(Op)) {
     MIB.addMBB(BBNode->getBasicBlock());
   } else if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Op)) {
     MIB.addFrameIndex(FI->getIndex());
   } else if (JumpTableSDNode *JT = dyn_cast<JumpTableSDNode>(Op)) {
     MIB.addJumpTableIndex(JT->getIndex(), JT->getTargetFlags());
   } else if (ConstantPoolSDNode *CP = dyn_cast<ConstantPoolSDNode>(Op)) {
     int Offset = CP->getOffset();
     unsigned Align = CP->getAlignment();
     Type *Type = CP->getType();

test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll

 ; RUN: llc -march=r600 -mcpu=redwood < %s | FileCheck -check-prefix=EG -check-prefix=FUNC %s
 
 ; FUNC-LABEL: {{^}}tgid_x:
 ; EG: MEM_RAT_CACHELESS STORE_RAW T1.X
 define amdgpu_kernel void @tgid_x(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tgid.x() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}tgid_y:
-; EG: MEM_RAT_CACHELESS STORE_RAW T1.Y
+; EG: MEM_RAT_CACHELESS STORE_RAW [[REG:T[0-9]+]].X
+; EG: MOV [[REG]].X, T1.Y
 define amdgpu_kernel void @tgid_y(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tgid.y() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}tgid_z:
-; EG: MEM_RAT_CACHELESS STORE_RAW T1.Z
+; EG: MEM_RAT_CACHELESS STORE_RAW [[REG:T[0-9]+]].X
+; EG: MOV [[REG]].X, T1.Z
 define amdgpu_kernel void @tgid_z(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tgid.z() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}tidig_x:
 ; EG: MEM_RAT_CACHELESS STORE_RAW T0.X
 define amdgpu_kernel void @tidig_x(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tidig.x() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}tidig_y:
-; EG: MEM_RAT_CACHELESS STORE_RAW T0.Y
+; EG: MEM_RAT_CACHELESS STORE_RAW [[REG:T[0-9]+]].X
+; EG: MOV [[REG]].X, T0.Y
 define amdgpu_kernel void @tidig_y(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tidig.y() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}tidig_z:
-; EG: MEM_RAT_CACHELESS STORE_RAW T0.Z
+; EG: MEM_RAT_CACHELESS STORE_RAW [[REG:T[0-9]+]].X
+; EG: MOV [[REG]].X, T0.Z
 define amdgpu_kernel void @tidig_z(i32 addrspace(1)* %out) {
 entry:
   %0 = call i32 @llvm.r600.read.tidig.z() #0
   store i32 %0, i32 addrspace(1)* %out
   ret void
 }
 
 ; FUNC-LABEL: {{^}}test_implicit:

test/CodeGen/Mips/tailcall/tailcall.ll

 ; RUN: llc -march=mipsel -relocation-model=pic  \
 ; RUN:     -verify-machineinstrs -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,PIC32
 ; RUN: llc -march=mipsel -relocation-model=static  \
 ; RUN:     -verify-machineinstrs -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,STATIC32
 ; RUN: llc -march=mips64el -mcpu=mips64r2  -relocation-model=pic \
 ; RUN:     -verify-machineinstrs -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,PIC64
 ; RUN: llc -march=mips64el -mcpu=mips64r2  -relocation-model=static \
 ; RUN:     -verify-machineinstrs -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,STATIC64
 ; RUN: llc -march=mipsel -mattr=mips16 -relocation-model=pic \
 ; RUN:      -verify-machineinstrs -mips-tail-calls=1 < %s | \
 ; RUN:     FileCheck %s -check-prefixes=ALL,PIC16
 
-; RUN: llc -march=mipsel -relocation-model=pic -mattr=+micromips -mips-tail-calls=1 < %s | \
-; RUN:     FileCheck %s -check-prefixes=ALL,PIC32MM
-; RUN: llc -march=mipsel -relocation-model=static -mattr=+micromips \
+; RUN: llc -march=mipsel -relocation-model=pic -mattr=+micromips -verify-machineinstrs \
+; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,PIC32MM
+; RUN: llc -march=mipsel -relocation-model=static -mattr=+micromips -verify-machineinstrs \
 ; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,STATIC32
 
-; RUN: llc -march=mipsel -relocation-model=pic -mcpu=mips32r6 -mips-tail-calls=1 < %s | \
-; RUN:     FileCheck %s -check-prefixes=ALL,PIC32R6
-; RUN: llc -march=mipsel -relocation-model=static -mcpu=mips32r2 \
+; RUN: llc -march=mipsel -relocation-model=pic -mcpu=mips32r6 -verify-machineinstrs \
+; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,PIC32R6
+; RUN: llc -march=mipsel -relocation-model=static -mcpu=mips32r2 -verify-machineinstrs \
 ; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,STATIC32
-; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r2  \
+; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r2 -verify-machineinstrs \
 ; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefix=PIC64
-; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r6  \
+; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r6 -verify-machineinstrs \
 ; RUN:     -mips-tail-calls=1 < %s | FileCheck %s -check-prefix=STATIC64
 
-; RUN: llc -march=mipsel -relocation-model=pic -mcpu=mips32r6 -mattr=+micromips \
+; RUN: llc -march=mipsel -relocation-model=pic -mcpu=mips32r6 -mattr=+micromips -verify-machineinstrs \
 ; RUN:      -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,PIC32MM
-; RUN: llc -march=mipsel -relocation-model=static -mcpu=mips32r6 \
+; RUN: llc -march=mipsel -relocation-model=static -mcpu=mips32r6 -verify-machineinstrs \
 ; RUN:     -mattr=+micromips -mips-tail-calls=1 < %s | FileCheck %s -check-prefixes=ALL,STATIC32MMR6
 
 @g0 = common global i32 0, align 4
 @g1 = common global i32 0, align 4
 @g2 = common global i32 0, align 4
 @g3 = common global i32 0, align 4
 @g4 = common global i32 0, align 4
 @g5 = common global i32 0, align 4