[TwoAddressInstruction] Tweak constraining of tied operands w/undef source

[reames]

Summary

This is a rework - and arguably a partial revert - of dff3454bda097723799935e8ea7f026ff0626940. That change modified the register constraint on an undef tied operand to use the regclass of the source operand. Before that change, we'd use the regclass of the operand from MCDesc.

From what I can tell from that patch, the intention was to handle generic target opcodes which don't have meaningful MCDesc. Unfortunately, the original test case appears to have been somewhat fragile. I can fully revert the original patch, and nothing changes today. So, I'm not 100% sure I actually understand it's intent.

This change uses the MCDesc regclass if one is available, and then falls back to the source operand if not. The motivation for this is that we can CSE together two instructions with distinct def regclasses, and end up with the source operand being a subset of the two. The result of this is an overly constrained source. Because the source is undef, we can relax it back to what the instruction definition required, and prevent redundant copies being inserted.

Note that in practice, the only case we appear to merge two instructions with distinct destination register classes are generic target opcodes. I hit this while working on a change in IMPLICIT_DEF handling, and have not been able to exercise it on ToT through anything but a carefully crafted MIR test.

[foad]

This is a rework - and arguably a partial revert - of dff3454bda097723799935e8ea7f026ff0626940. That change modified the register constraint on an undef tied operand to use the regclass of the source operand. Before that change, we'd use the regclass of the operand from MCDesc.

From what I can tell from that patch, the intention was to handle generic target opcodes which don't have meaningful MCDesc.

Quoting the description of D110944:

before:

%16:sgpr_96 = INSERT_SUBREG undef %15:sgpr_96_with_sub0_sub1(tied-def 0), killed %11:sreg_64_xexec, %subreg.sub0_sub1

After, without this patch:

undef %16.sub0_sub1:sgpr_96 = COPY killed %11:sreg_64_xexec

The problem was that sgpr_96 does not fully support sub0_sub1. Something should have constrained that regclass to sgpr_96_with_sub0_sub1 somehow, e.g. by calling getSubClassWithSubReg. My approach was to use the class of the tied src operand to constrain it.

Unfortunately, the original test case appears to have been somewhat fragile. I can fully revert the original patch, and nothing changes today. So, I'm not 100% sure I actually understand it's intent.

I suspect the original repro no longer applies since D151463 changed the way AMDGPU's sgpr_96 register classes are defined.

llvm/test/CodeGen/RISCV/twoaddress-undef-regclass-constraint.mir

1

Can you precommit this or split it out to a separate patch, so we can see the diff?

[foad]

llvm/test/CodeGen/RISCV/twoaddress-undef-regclass-constraint.mir

100

Do you have a realistic example of where the :vrnov0 class might have come from in the first place? That might help explain why it's OK to ignore it.

[foad]

Just trying to clarify my own thoughts:

• I have no objection to your patch, though I do wonder if we can simplify this code even more.
• The specific AMDGPU case that prompted D110944 no longer exists, and if it or anything like it comes back, we can re-examine it afresh.
• If I was trying to fix that AMDGPU case today I think I would do it differently anyway: the part of TwoAddressInstructionPass::runOnMachineFunction that rewrites INSERT_SUBREG as COPY should probably just call getSubClassWithSubReg to constrain the dst operand when it sets the subreg.

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

tryInstructionTransform(MachineBasicBlock::iterator &mi,
   return false;
 }
 
 // Collect tied operands of MI that need to be handled.
 // Rewrite trivial cases immediately.
 // Return true if any tied operands where found, including the trivial ones.
 bool TwoAddressInstructionPass::
 collectTiedOperands(MachineInstr *MI, TiedOperandMap &TiedOperands) {
+  const MCInstrDesc &MCID = MI->getDesc();
   bool AnyOps = false;
   unsigned NumOps = MI->getNumOperands();
 
   for (unsigned SrcIdx = 0; SrcIdx < NumOps; ++SrcIdx) {
     unsigned DstIdx = 0;
     if (!MI->isRegTiedToDefOperand(SrcIdx, &DstIdx))
       continue;
     AnyOps = true;
     MachineOperand &SrcMO = MI->getOperand(SrcIdx);
     MachineOperand &DstMO = MI->getOperand(DstIdx);
     Register SrcReg = SrcMO.getReg();
     Register DstReg = DstMO.getReg();
     // Tied constraint already satisfied?
     if (SrcReg == DstReg)
       continue;
 
     assert(SrcReg && SrcMO.isUse() && "two address instruction invalid");
 
     // Deal with undef uses immediately - simply rewrite the src operand.
     if (SrcMO.isUndef() && !DstMO.getSubReg()) {
-      // Constrain the DstReg register class if required.
+      // Constrain the DstReg register class if required.  We need the
+      // tied register to satisify the constraints of both source and dest.
+      // For some target independent opcodes, we don't have a regclass in
+      // the MCID, and have to use the actual regclass of the source.
+      // This may be much more restrictive than actually required.
       if (DstReg.isVirtual()) {
-        const TargetRegisterClass *RC = MRI->getRegClass(SrcReg);
+        const TargetRegisterClass *RC = TII->getRegClass(MCID, SrcIdx,
+                                                         TRI, *MF);
+        if (!RC)
+          RC = MRI->getRegClass(SrcReg);
         MRI->constrainRegClass(DstReg, RC);
       }
       SrcMO.setReg(DstReg);
       SrcMO.setSubReg(0);
       LLVM_DEBUG(dbgs() << "\t\trewrite undef:\t" << *MI);
       continue;
     }
     TiedOperands[SrcReg].push_back(std::make_pair(SrcIdx, DstIdx));

llvm/test/CodeGen/RISCV/twoaddress-undef-regclass-constraint.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv32 -mattr=+d,+zfh,+zvfh,+v,+m -target-abi=ilp32d -run-pass twoaddressinstruction -run-pass machineverifier %s -o - | FileCheck %s
+
+--- |
+  source_filename = "<stdin>"
+  target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
+  target triple = "riscv32"
+
+  define <2 x i64> @vp_bitreverse_v2i64() #0 {
+    ret <2 x i64> zeroinitializer
+  }
+
+  attributes #0 = { "target-features"="+d,+zfh,+zvfh,+v,+m" }
+
+...
+---
+name:            vp_bitreverse_v2i64
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+callsEHReturn:   false
+callsUnwindInit: false
+hasEHCatchret:   false
+hasEHScopes:     false
+hasEHFunclets:   false
+isOutlined:      false
+debugInstrRef:   false
+failsVerification: false
+tracksDebugUserValues: false
+registers:
+  - { id: 0, class: vr, preferred-register: '' }
+  - { id: 1, class: vr, preferred-register: '' }
+  - { id: 2, class: gprnox0, preferred-register: '' }
+  - { id: 5, class: vrnov0, preferred-register: '' }
+  - { id: 17, class: vr, preferred-register: '' }
+  - { id: 18, class: vr, preferred-register: '' }
+  - { id: 19, class: vr, preferred-register: '' }
+  - { id: 20, class: vr, preferred-register: '' }
+  - { id: 21, class: vr, preferred-register: '' }
+  - { id: 22, class: vr, preferred-register: '' }
+  - { id: 23, class: vr, preferred-register: '' }
+  - { id: 24, class: gpr, preferred-register: '' }
+  - { id: 25, class: vrnov0, preferred-register: '' }
+liveins:
+  - { reg: '$v8', virtual-reg: '%0' }
+  - { reg: '$v0', virtual-reg: '%1' }
+  - { reg: '$x10', virtual-reg: '%2' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    1
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  functionContext: ''
+  maxCallFrameSize: 4294967295
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:           []
+entry_values:    []
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo:
+  varArgsFrameIndex: 0
+  varArgsSaveSize: 0
+body:             |
+  bb.0 (%ir-block.0):
+    liveins: $v8, $v0, $x10
+
+
+    ; CHECK-LABEL: name: vp_bitreverse_v2i64
+    ; CHECK: liveins: $v8, $v0, $x10
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: dead $x0 = PseudoVSETIVLI 4, 208 /* e32, m1, ta, ma */, implicit-def $vl, implicit-def $vtype
+    ; CHECK-NEXT: [[PseudoVMV_V_I_M1_:%[0-9]+]]:vr = PseudoVMV_V_I_M1 undef [[PseudoVMV_V_I_M1_]], 0, 4, 5 /* e32 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+    ; CHECK-NEXT: [[PseudoVMV_V_I_MF8_:%[0-9]+]]:vr = PseudoVMV_V_I_MF8 undef [[PseudoVMV_V_I_MF8_]], 5, 1, 3 /* e8 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+    ; CHECK-NEXT: [[LUI:%[0-9]+]]:gpr = LUI 1044480
+    ; CHECK-NEXT: $v0 = COPY killed [[PseudoVMV_V_I_MF8_]]
+    ; CHECK-NEXT: [[PseudoVMERGE_VXM_M1_:%[0-9]+]]:vrnov0 = PseudoVMERGE_VXM_M1 undef [[PseudoVMERGE_VXM_M1_]], killed [[PseudoVMV_V_I_M1_]], killed [[LUI]], killed $v0, 4, 5 /* e32 */, implicit $vl, implicit $vtype
+    ; CHECK-NEXT: $v8 = COPY killed [[PseudoVMERGE_VXM_M1_]]
+    ; CHECK-NEXT: PseudoRET implicit killed $v8
+    dead $x0 = PseudoVSETIVLI 4, 208 /* e32, m1, ta, ma */, implicit-def $vl, implicit-def $vtype
+    %17:vr = PseudoVMV_V_I_M1 undef %5:vrnov0, 0, 4, 5 /* e32 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+    %19:vr = PseudoVMV_V_I_MF8 undef %5:vrnov0, 5, 1, 3 /* e8 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+    %24:gpr = LUI 1044480
+    $v0 = COPY killed %19
+    %25:vrnov0 = PseudoVMERGE_VXM_M1 undef %5:vrnov0, killed %17, killed %24, killed $v0, 4, 5 /* e32 */, implicit $vl, implicit $vtype
+    $v8 = COPY killed %25
+    PseudoRET implicit killed $v8
+
+...