[GISel]: Don't assert when constraining Registers which are uses if there's no regclass.

[aditya_nandakumar]

Summary

Currently we assert that only non target specific opcodes can have missing RegisterClass constraints in the MCDesc. The backend can have instructions with register operands but don't have RegisterClass constraints (say using unknown_tuple) in which case the instruction defining the register will constrain it.
Change the assert to only fire if a def has no regclass.

[dsanders]

There isn't an unknown_tuple in tree. I think you're referring to unknown_class and unknown which have only been used by AMDGPU so far. Is that correct?

[aditya_nandakumar]

In D43409#1010788, @dsanders wrote:

There isn't an unknown_tuple in tree. I think you're referring to unknown_class and unknown which have only been used by AMDGPU so far. Is that correct?

Thanks Daniel. That's what I meant (clearly I got confused with names).

[dsanders]

Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

[aditya_nandakumar]

In D43409#1014897, @dsanders wrote:

Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

[qcolombet]

LGTM.

Any chance you could add a test case?

[aditya_nandakumar]

In D43409#1019378, @qcolombet wrote:

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

[dsanders]

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

In D43409#1019419, @aditya_nandakumar wrote:

In D43409#1019378, @qcolombet wrote:

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

lib/CodeGen/GlobalISel/Utils.cpp

62–63

It could be clearer that 'unless they are uses' refers to the operands.

[aditya_nandakumar]

In D43409#1019482, @dsanders wrote:

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

In D43409#1019419, @aditya_nandakumar wrote:

In D43409#1019378, @qcolombet wrote:

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

I don't think AMDGPU imports any rules from what I see.

[dsanders]

In D43409#1019714, @aditya_nandakumar wrote:

In D43409#1019482, @dsanders wrote:

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

In D43409#1019419, @aditya_nandakumar wrote:

In D43409#1019378, @qcolombet wrote:

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

I don't think AMDGPU imports any rules from what I see.

In that case I don't think this is test-able in-tree. LGTM with just the nit

[aditya_nandakumar]

Pushed in 326142

include/llvm/CodeGen/GlobalISel/Utils.h

 #define LLVM_CODEGEN_GLOBALISEL_UTILS_H
 
 #include "llvm/ADT/StringRef.h"
 
 namespace llvm {
 
 class MachineFunction;
 class MachineInstr;
+class MachineOperand;
 class MachineOptimizationRemarkEmitter;
 class MachineOptimizationRemarkMissed;
 class MachineRegisterInfo;
 class MCInstrDesc;
 class RegisterBankInfo;
 class TargetInstrInfo;
 class TargetPassConfig;
 class TargetRegisterInfo;

 ///
 /// \return The virtual register constrained to the right register class.
 unsigned constrainOperandRegClass(const MachineFunction &MF,
                                   const TargetRegisterInfo &TRI,
                                   MachineRegisterInfo &MRI,
                                   const TargetInstrInfo &TII,
                                   const RegisterBankInfo &RBI,
                                   MachineInstr &InsertPt, const MCInstrDesc &II,
-                                  unsigned Reg, unsigned OpIdx);
+                                  const MachineOperand &RegMO, unsigned OpIdx);
 
 /// Mutate the newly-selected instruction \p I to constrain its (possibly
 /// generic) virtual register operands to the instruction's register class.
 /// This could involve inserting COPYs before (for uses) or after (for defs).
 /// This requires the number of operands to match the instruction description.
 /// \returns whether operand regclass constraining succeeded.
 ///
 // FIXME: Not all instructions have the same number of operands. We should

lib/CodeGen/GlobalISel/Utils.cpp

BuildMI(*InsertPt.getParent(), InsertPt, InsertPt.getDebugLoc(),
             TII.get(TargetOpcode::COPY), NewReg)
         .addReg(Reg);
     return NewReg;
   }
 
   return Reg;
 }
 
-
 unsigned llvm::constrainOperandRegClass(
     const MachineFunction &MF, const TargetRegisterInfo &TRI,
     MachineRegisterInfo &MRI, const TargetInstrInfo &TII,
     const RegisterBankInfo &RBI, MachineInstr &InsertPt, const MCInstrDesc &II,
-    unsigned Reg, unsigned OpIdx) {
+    const MachineOperand &RegMO, unsigned OpIdx) {
+  unsigned Reg = RegMO.getReg();
   // Assume physical registers are properly constrained.
   assert(TargetRegisterInfo::isVirtualRegister(Reg) &&
          "PhysReg not implemented");
 
   const TargetRegisterClass *RegClass = TII.getRegClass(II, OpIdx, &TRI, MF);
   // Some of the target independent instructions, like COPY, may not impose any
-  // register class constraints on some of their operands:
+  // register class constraints on some of their operands: If it's a use, we can
+  // skip constraining as the instruction defining the register would constrain
+  // it.
   if (!RegClass) {
-    assert(!isTargetSpecificOpcode(II.getOpcode()) &&
+    assert((!isTargetSpecificOpcode(II.getOpcode()) || RegMO.isUse()) &&
            "Only target independent instructions are allowed to have operands "
-           "with no register class constraints");
+           "with no register class constraints unless they are uses");
     // FIXME: Just bailing out like this here could be not enough, unless we
     // expect the users of this function to do the right thing for PHIs and
     // COPY:
     //   v1 = COPY v0
     //   v2 = COPY v1
     // v1 here may end up not being constrained at all. Please notice that to
     // reproduce the issue we likely need a destination pattern of a selection
     // rule producing such extra copies, not just an input GMIR with them as
for (unsigned OpI = 0, OpE = I.getNumExplicitOperands(); OpI != OpE; ++OpI) {
     // to be constrained.
     if (Reg == 0)
       continue;
 
     // If the operand is a vreg, we should constrain its regclass, and only
     // insert COPYs if that's impossible.
     // constrainOperandRegClass does that for us.
     MO.setReg(constrainOperandRegClass(MF, TRI, MRI, TII, RBI, I, I.getDesc(),
-                                       Reg, OpI));
+                                       MO, OpI));
 
     // Tie uses to defs as indicated in MCInstrDesc if this hasn't already been
     // done.
     if (MO.isUse()) {
       int DefIdx = I.getDesc().getOperandConstraint(OpI, MCOI::TIED_TO);
       if (DefIdx != -1 && !I.isRegTiedToUseOperand(DefIdx))
         I.tieOperands(DefIdx, OpI);
     }

lib/Target/AArch64/AArch64CallLowering.cpp

bool AArch64CallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
   MIRBuilder.insertInstr(MIB);
 
   // If Callee is a reg, since it is used by a target specific
   // instruction, it must have a register class matching the
   // constraint of that instruction.
   if (Callee.isReg())
     MIB->getOperand(0).setReg(constrainOperandRegClass(
         MF, *TRI, MRI, *MF.getSubtarget().getInstrInfo(),
-        *MF.getSubtarget().getRegBankInfo(), *MIB, MIB->getDesc(),
-        Callee.getReg(), 0));
+        *MF.getSubtarget().getRegBankInfo(), *MIB, MIB->getDesc(), Callee, 0));
 
   // Finally we can copy the returned value back into its virtual-register. In
   // symmetry with the arugments, the physical register must be an
   // implicit-define of the call instruction.
   CCAssignFn *RetAssignFn = TLI.CCAssignFnForReturn(F.getCallingConv());
   if (OrigRet.Reg) {
     SplitArgs.clear();
 

lib/Target/ARM/ARMCallLowering.cpp

bool ARMCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
   auto MIB = MIRBuilder.buildInstrNoInsert(CallOpcode)
                  .add(Callee)
                  .addRegMask(TRI->getCallPreservedMask(MF, CallConv));
   if (Callee.isReg()) {
     auto CalleeReg = Callee.getReg();
     if (CalleeReg && !TRI->isPhysicalRegister(CalleeReg))
       MIB->getOperand(0).setReg(constrainOperandRegClass(
           MF, *TRI, MRI, *STI.getInstrInfo(), *STI.getRegBankInfo(),
-          *MIB.getInstr(), MIB->getDesc(), CalleeReg, 0));
+          *MIB.getInstr(), MIB->getDesc(), Callee, 0));
   }
 
   SmallVector<ArgInfo, 8> ArgInfos;
   for (auto Arg : OrigArgs) {
     if (!isSupportedType(DL, TLI, Arg.Ty))
       return false;
 
     if (!Arg.IsFixed)

lib/Target/X86/X86CallLowering.cpp

bool X86CallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
   MIRBuilder.insertInstr(MIB);
 
   // If Callee is a reg, since it is used by a target specific
   // instruction, it must have a register class matching the
   // constraint of that instruction.
   if (Callee.isReg())
     MIB->getOperand(0).setReg(constrainOperandRegClass(
         MF, *TRI, MRI, *MF.getSubtarget().getInstrInfo(),
-        *MF.getSubtarget().getRegBankInfo(), *MIB, MIB->getDesc(),
-        Callee.getReg(), 0));
+        *MF.getSubtarget().getRegBankInfo(), *MIB, MIB->getDesc(), Callee, 0));
 
   // Finally we can copy the returned value back into its virtual-register. In
   // symmetry with the arguments, the physical register must be an
   // implicit-define of the call instruction.
 
   if (OrigRet.Reg) {
     SplitArgs.clear();
     SmallVector<unsigned, 8> NewRegs;