[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

408 ↗

(On Diff #116164)

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 ↗

(On Diff #127351)

This comment should go above:

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

330 ↗

(On Diff #127351)

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 ↗

(On Diff #133193)

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

[smaksimovic]

rL324733

lib/Target/Mips/MipsSEISelDAGToDAG.h

 private:
 
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   void addDSPCtrlRegOperands(bool IsDef, MachineInstr &MI,
                              MachineFunction &MF);
   
+  void addCOPYNodeSW16_MM(MachineFunction &MF, MachineInstr &MI);
+
   unsigned getMSACtrlReg(const SDValue RegIdx) const;
 
   bool replaceUsesWithZeroReg(MachineRegisterInfo *MRI, const MachineInstr&);
 
   std::pair<SDNode *, SDNode *> selectMULT(SDNode *N, unsigned Opc,
                                            const SDLoc &dl, EVT Ty, bool HasLo,
                                            bool HasHi);
 

lib/Target/Mips/MipsSEISelDAGToDAG.cpp

bool MipsSEDAGToDAGISel::runOnMachineFunction(MachineFunction &MF) {
   return MipsDAGToDAGISel::runOnMachineFunction(MF);
 }
 
 void MipsSEDAGToDAGISel::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<DominatorTreeWrapperPass>();
   SelectionDAGISel::getAnalysisUsage(AU);
 }
 
+void
+MipsSEDAGToDAGISel::addCOPYNodeSW16_MM(MachineFunction &MF, MachineInstr &MI) {
+  MachineBasicBlock &MBB = *MI.getParent();
+  const TargetRegisterInfo *TRI = Subtarget->getRegisterInfo();
+  const TargetInstrInfo *TII = Subtarget->getInstrInfo();
+  MachineRegisterInfo &RegInfo = MF.getRegInfo();
+  DebugLoc DL = MI.getDebugLoc();
+  MachineBasicBlock::iterator II(MI);
+
+  unsigned Reg = MI.getOperand(0).getReg();
+  const auto &GPRMM16ZeroRegClass =
+      TRI->getRegClass(Mips::GPRMM16ZeroRegClassID);
+  unsigned NewReg = RegInfo.createVirtualRegister(GPRMM16ZeroRegClass);
+
+  BuildMI(MBB, II, DL, TII->get(Mips::COPY), NewReg).addReg(Reg);
+  
+  MI.getOperand(0).setReg(NewReg);
+}
+
 void MipsSEDAGToDAGISel::addDSPCtrlRegOperands(bool IsDef, MachineInstr &MI,
                                                MachineFunction &MF) {
   MachineInstrBuilder MIB(MF, &MI);
   unsigned Mask = MI.getOperand(1).getImm();
   unsigned Flag =
       IsDef ? RegState::ImplicitDefine : RegState::Implicit | RegState::Undef;
 
   if (Mask & 1)
for (auto &MBB: MF) {
     for (auto &MI: MBB) {
       switch (MI.getOpcode()) {
       case Mips::RDDSP:
         addDSPCtrlRegOperands(false, MI, MF);
         break;
       case Mips::WRDSP:
         addDSPCtrlRegOperands(true, MI, MF);
         break;
+      case Mips::SW16_MM:
+        addCOPYNodeSW16_MM(MF, MI);
+        break;
       default:
         replaceUsesWithZeroReg(MRI, MI);
       }
     }
   }
 }
 
 void MipsSEDAGToDAGISel::selectAddE(SDNode *Node, const SDLoc &DL) const {

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 -mips-tail-calls=1 < %s \
+; RUN:     -verify-machineinstrs | 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,STATIC32
-; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r6 \
+; RUN: llc -march=mips64el -relocation-model=pic -mcpu=mips64r6 -verify-machineinstrs \
 ; RUN:     -mattr=+micromips -mips-tail-calls=1 < %s | FileCheck %s -check-prefix=PIC64R6MM
-; RUN: llc -march=mips64el -relocation-model=static -mcpu=mips64r6 \
+; RUN: llc -march=mips64el -relocation-model=static -mcpu=mips64r6 -verify-machineinstrs \
 ; RUN:     -mattr=+micromips -mips-tail-calls=1 < %s | FileCheck %s -check-prefix=STATIC64
 
 @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