[SPIR-V] Begin removing explicit setRegClass calls

[zuban32]

Summary

As a part of a general cleanup of SPIRV's instruction selection
we want to remove explicit setRegClass calls which happen in some
SPIRV-specific pre-selection lowering passes. As these calls only
concern finally emitted SPIRV instructions and not MIR ones we can
easily constrain them.

This change removes the calls everywhere except for the builtins-
related code, that will require more effort to refactor the overall
bultins emission code.

[iliya-diyachkov]

Could you please get rid of these calls to constrainSelectedInstRegOperands?

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

321–322

constrainSelectedInstRegOperands should be called from InstructionSelector only. We also have it in methods of SPIRVGlobalRegistry but these methods are called from InstructionSelector.

334

The same issue with constrainSelectedInstRegOperands.

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp

737

It can be the issue with constrainSelectedInstRegOperands if it's called outside InstructionSelector.

986

It can be the issue with constrainSelectedInstRegOperands if it's called outside InstructionSelector.

llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp

217

The same issue with constrainSelectedInstRegOperands.

344

The same issue with constrainSelectedInstRegOperands.

[mpaszkowski]

There are some new regressions after this individual patch, we are hitting an assert in SPIRVInstructionSelector for example in integer_ops / long_logic. Full testing results: https://spirv-testing.khronos.org/job/142/

test_integer_ops: /home/bot/Workspace/llvm-monorepo/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:228: virtual bool {anonymous}::SPIRVInstructionSelector::select(llvm::MachineInstr&): Assertion `Res || Def->getOpcode() == TargetOpcode::G_CONSTANT' failed.

[zuban32]

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

321–322

This is the whole point of the patch - these instructions are built before selection, but we need to have them constrained in order to support selection of instruction that depend on them (like G_ADD adding two OpFunctionParameter's). Using setRegClass explicitly is wrong since these classes are already described by the instruction description in tablegen. I remember @arsenm pointed out the problem a while ago.

@iliya-diyachkov do you see any particular issue with calling these funcs from here? I see there're a couple of non-selector uses in AMDGPU RegBankSelect and MIPS legalizer. Anyway, I don't see any viable alternative.

[iliya-diyachkov]

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

321–322

@zuban32 no particular problem, except that this is the wrong place to call this interface. As you write, you are going to continue removing setRegClass in builtins-related code, so you will need to insert much more calls to constrainSelectedInstRegOperands outside of InstructionSelector.

Am I right that D153101 patch (with a small fix) can be applied without this one? In my sandbox, it passes all lit tests. If so, it would be nice to test it on OpenCL CTS.

I understand that using setRegClass is logically excessive (and may look unsightly) since we have the info in the instruction description, however (as I see it) other targets also use setRegClass in this way.

On other hand, as we discussed earlier, we are going to exclude "global" instructions from the function level. Many instructions (which require setRegClass) are "global", so they will go away from IR with no need to call constrainSelectedInstRegOperands outside of InstructionSelector. Generation of others (mainly in SPIRVBuiltins.cpp) can probably be moved to InstructionSelector. After this, we will have a few instructions that require setting of reg class before InstructionSelector (like in AMDGPU/MIPS), which is acceptable.

So, I propose either to postpone this patch and commit other two patches (and continue the plan described before), or to mark all inserted constrainSelectedInstRegOperands calls with "FIXME" comment with the obligation to remove them when we find a better solution, and try not to spread such use of constrainSelectedInstRegOperands in the next patches.

[zuban32]

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

321–322

@zuban32 no particular problem, except that this is the wrong place to call this interface

Why particularly? If you are aware of the exact issue this can cause pls share that

. As you write, you are going to continue removing setRegClass in builtins-related code, so you will need to insert much more calls to constrainSelectedInstRegOperands outside of InstructionSelector.

That's right, and that was supposed to be the aim of my subsequent patches.

Am I right that D153101 patch (with a small fix) can be applied without this one? In my sandbox, it passes all lit tests. If so, it would be nice to test it on OpenCL CTS.

I understand that using setRegClass is logically excessive (and may look unsightly) since we have the info in the instruction description, however (as I see it) other targets also use setRegClass in this way.

From what I see these targets are doing that mainly during or after selection.

On other hand, as we discussed earlier, we are going to exclude "global" instructions from the function level. Many instructions (which require setRegClass) are "global", so they will go away from IR with no need to call constrainSelectedInstRegOperands outside of InstructionSelector. Generation of others (mainly in SPIRVBuiltins.cpp) can probably be moved to InstructionSelector. After this, we will have a few instructions that require setting of reg class before InstructionSelector (like in AMDGPU/MIPS), which is acceptable.

I think the problem that I was trying to solve is to avoid this 'manual' regclass setting (which BTW sometimes led to some verifier errors, I don't recall the exact usecase, I'll find it and post here) by leaving it all to the automated utilities based on the instruction description that already exists.

So, I propose either to postpone this patch and commit other two patches (and continue the plan described before), or to mark all inserted constrainSelectedInstRegOperands calls with "FIXME" comment with the obligation to remove them when we find a better solution, and try not to spread such use of constrainSelectedInstRegOperands in the next patches.

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

for (const auto &Arg : F.args()) {
       }
       ++i;
     }
   }
 
   // Generate a SPIR-V type for the function.
   auto MRI = MIRBuilder.getMRI();
   Register FuncVReg = MRI->createGenericVirtualRegister(LLT::scalar(32));
-  MRI->setRegClass(FuncVReg, &SPIRV::IDRegClass);
   if (F.isDeclaration())
     GR->add(&F, &MIRBuilder.getMF(), FuncVReg);
   SPIRVType *RetTy = GR->getOrCreateSPIRVType(FTy->getReturnType(), MIRBuilder);
   SPIRVType *FuncTy = GR->getOrCreateOpTypeFunctionWithArgs(
       FTy, RetTy, ArgTypeVRegs, MIRBuilder);
 
   // Build the OpTypeFunction declaring it.
   uint32_t FuncControl = getFunctionControl(F);
 
-  MIRBuilder.buildInstr(SPIRV::OpFunction)
+  auto B = MIRBuilder.buildInstr(SPIRV::OpFunction)
                .addDef(FuncVReg)
                .addUse(GR->getSPIRVTypeID(RetTy))
                .addImm(FuncControl)
                .addUse(GR->getSPIRVTypeID(FuncTy));
+  const auto &ST = GR->CurMF->getSubtarget();
+  constrainSelectedInstRegOperands(*B.getInstr(), *ST.getInstrInfo(),
+                                   *ST.getRegisterInfo(), *ST.getRegBankInfo());
 
   // Add OpFunctionParameters.
   int i = 0;
   for (const auto &Arg : F.args()) {
     assert(VRegs[i].size() == 1 && "Formal arg has multiple vregs");
-    MRI->setRegClass(VRegs[i][0], &SPIRV::IDRegClass);
-    MIRBuilder.buildInstr(SPIRV::OpFunctionParameter)
+    auto B = MIRBuilder.buildInstr(SPIRV::OpFunctionParameter)
                  .addDef(VRegs[i][0])
                  .addUse(GR->getSPIRVTypeID(ArgTypeVRegs[i]));
     if (F.isDeclaration())
       GR->add(&Arg, &MIRBuilder.getMF(), VRegs[i][0]);
+    const auto &ST = GR->CurMF->getSubtarget();
+    constrainSelectedInstRegOperands(*B.getInstr(), *ST.getInstrInfo(),
+                                     *ST.getRegisterInfo(),
+                                     *ST.getRegBankInfo());
     i++;
   }
   // Name the function.
   if (F.hasName())
     buildOpName(FuncVReg, F.getName(), MIRBuilder);
 
   // Handle entry points and function linkage.
   if (F.getCallingConv() == CallingConv::SPIR_KERNEL) {
if (CF && CF->isDeclaration() &&
     FirstBlockBuilder.setMBB(*MF.getBlockNumbered(0));
 
     SmallVector<ArrayRef<Register>, 8> VRegArgs;
     SmallVector<SmallVector<Register, 1>, 8> ToInsert;
     for (const Argument &Arg : CF->args()) {
       if (MIRBuilder.getDataLayout().getTypeStoreSize(Arg.getType()).isZero())
         continue; // Don't handle zero sized types.
       Register Reg = MRI->createGenericVirtualRegister(LLT::scalar(32));
-      MRI->setRegClass(Reg, &SPIRV::IDRegClass);
       ToInsert.push_back({Reg});
       VRegArgs.push_back(ToInsert.back());
     }
     // TODO: Reuse FunctionLoweringInfo
     FunctionLoweringInfo FuncInfo;
     lowerFormalArguments(FirstBlockBuilder, *CF, VRegArgs, FuncInfo);
   }
 
   // Make sure there's a valid return reg, even for functions returning void.
   if (!ResVReg.isValid())
-    ResVReg = MIRBuilder.getMRI()->createVirtualRegister(&SPIRV::IDRegClass);
+    ResVReg =
+        MIRBuilder.getMRI()->createGenericVirtualRegister(LLT::scalar(32));
   SPIRVType *RetType =
       GR->assignTypeToVReg(FTy->getReturnType(), ResVReg, MIRBuilder);
 
   // Emit the OpFunctionCall and its args.
   auto MIB = MIRBuilder.buildInstr(SPIRV::OpFunctionCall)
                  .addDef(ResVReg)
                  .addUse(GR->getSPIRVTypeID(RetType))
                  .add(Info.Callee);

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp

 
 SPIRVType *SPIRVGlobalRegistry::restOfCreateSPIRVType(
     const Type *Ty, MachineIRBuilder &MIRBuilder,
     SPIRV::AccessQualifier::AccessQualifier AccessQual, bool EmitIR) {
   if (TypesInProcessing.count(Ty) && !Ty->isPointerTy())
     return nullptr;
   TypesInProcessing.insert(Ty);
   SPIRVType *SpirvType = createSPIRVType(Ty, MIRBuilder, AccessQual, EmitIR);
+  const auto &ST = CurMF->getSubtarget();
+  constrainSelectedInstRegOperands(*const_cast<MachineInstr *>(SpirvType),
+                                   *ST.getInstrInfo(), *ST.getRegisterInfo(),
+                                   *ST.getRegBankInfo());
   TypesInProcessing.erase(Ty);
   VRegToTypeMap[&MIRBuilder.getMF()][getSPIRVTypeID(SpirvType)] = SpirvType;
   SPIRVToLLVMType[SpirvType] = Ty;
   Register Reg = DT.find(Ty, &MIRBuilder.getMF());
   // Do not add OpTypeForwardPointer to DT, a corresponding normal pointer type
   // will be added later. For special types it is already added to DT.
   if (SpirvType->getOpcode() != SPIRV::OpTypeForwardPointer && !Reg.isValid() &&
       !isSpecialOpaqueType(Ty))

 }
 
 SPIRVType *SPIRVGlobalRegistry::finishCreatingSPIRVType(const Type *LLVMTy,
                                                         SPIRVType *SpirvType) {
   assert(CurMF == SpirvType->getMF());
   VRegToTypeMap[CurMF][getSPIRVTypeID(SpirvType)] = SpirvType;
   SPIRVToLLVMType[SpirvType] = LLVMTy;
   DT.add(LLVMTy, CurMF, getSPIRVTypeID(SpirvType));
+  const auto &ST = CurMF->getSubtarget();
+  constrainSelectedInstRegOperands(*const_cast<MachineInstr *>(SpirvType),
+                                   *ST.getInstrInfo(), *ST.getRegisterInfo(),
+                                   *ST.getRegBankInfo());
   return SpirvType;
 }
 
 SPIRVType *SPIRVGlobalRegistry::getOrCreateSPIRVIntegerType(
     unsigned BitWidth, MachineInstr &I, const SPIRVInstrInfo &TII) {
   Type *LLVMTy = IntegerType::get(CurMF->getFunction().getContext(), BitWidth);
   Register Reg = DT.find(LLVMTy, CurMF);
   if (Reg.isValid())

llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp

for (MachineInstr &MI : MBB) {
         }
       }
     }
   }
   for (MachineInstr *MI : ToErase) {
     Register Reg = MI->getOperand(2).getReg();
     if (RegsAlreadyAddedToDT.find(MI) != RegsAlreadyAddedToDT.end())
       Reg = RegsAlreadyAddedToDT[MI];
-    auto *RC = MRI.getRegClassOrNull(MI->getOperand(0).getReg());
-    if (!MRI.getRegClassOrNull(Reg) && RC)
-      MRI.setRegClass(Reg, RC);
     MRI.replaceRegWith(MI->getOperand(0).getReg(), Reg);
     MI->eraseFromParent();
   }
   for (MachineInstr *MI : ToEraseComposites)
     MI->eraseFromParent();
 }
 
 static void foldConstantsIntoIntrinsics(MachineFunction &MF) {
if (!SpirvTy) {
           SpirvTy = propagateSPIRVType(Def, GR, MRI, MIB);
         break;
       }
       default:
         break;
       }
       if (SpirvTy)
         GR->assignSPIRVTypeToVReg(SpirvTy, Reg, MIB.getMF());
-      if (!MRI.getRegClassOrNull(Reg))
-        MRI.setRegClass(Reg, &SPIRV::IDRegClass);
     }
   }
   return SpirvTy;
 }
 
 // Insert ASSIGN_TYPE instuction between Reg and its definition, set NewReg as
 // a dst of the definition, assign SPIRVType to both registers. If SpirvTy is
 // provided, use it as SPIRVType in ASSIGN_TYPE, otherwise create it from Ty.
 // It's used also in SPIRVBuiltins.cpp.
 // TODO: maybe move to SPIRVUtils.
 namespace llvm {
 Register insertAssignInstr(Register Reg, Type *Ty, SPIRVType *SpirvTy,
                            SPIRVGlobalRegistry *GR, MachineIRBuilder &MIB,
                            MachineRegisterInfo &MRI) {
   MachineInstr *Def = MRI.getVRegDef(Reg);
   assert((Ty || SpirvTy) && "Either LLVM or SPIRV type is expected.");
   MIB.setInsertPt(*Def->getParent(),
                   (Def->getNextNode() ? Def->getNextNode()->getIterator()
                                       : Def->getParent()->end()));
   Register NewReg = MRI.createGenericVirtualRegister(MRI.getType(Reg));
-  if (auto *RC = MRI.getRegClassOrNull(Reg)) {
-    MRI.setRegClass(NewReg, RC);
-  } else {
-    MRI.setRegClass(NewReg, &SPIRV::IDRegClass);
-    MRI.setRegClass(Reg, &SPIRV::IDRegClass);
-  }
   SpirvTy = SpirvTy ? SpirvTy : GR->getOrCreateSPIRVType(Ty, MIB);
   GR->assignSPIRVTypeToVReg(SpirvTy, Reg, MIB.getMF());
   // This is to make it convenient for Legalizer to get the SPIRVType
   // when processing the actual MI (i.e. not pseudo one).
   GR->assignSPIRVTypeToVReg(SpirvTy, NewReg, MIB.getMF());
   // Copy MIFlags from Def to ASSIGN_TYPE instruction. It's required to keep
   // the flags after instruction selection.
   const uint32_t Flags = Def->getFlags();
-  MIB.buildInstr(SPIRV::ASSIGN_TYPE)
+  auto B = MIB.buildInstr(SPIRV::ASSIGN_TYPE)
                .addDef(Reg)
                .addUse(NewReg)
                .addUse(GR->getSPIRVTypeID(SpirvTy))
                .setMIFlags(Flags);
   Def->getOperand(0).setReg(NewReg);
+  const auto &ST = GR->CurMF->getSubtarget();
+  constrainSelectedInstRegOperands(*B.getInstr(), *ST.getInstrInfo(),
+                                   *ST.getRegisterInfo(), *ST.getRegBankInfo());
   return NewReg;
 }
 } // namespace llvm
 
 static void generateAssignInstrs(MachineFunction &MF, SPIRVGlobalRegistry *GR,
                                  MachineIRBuilder MIB) {
   MachineRegisterInfo &MRI = MF.getRegInfo();
   SmallVector<MachineInstr *, 10> ToErase;
createNewIdReg(Register ValReg, unsigned Opcode, MachineRegisterInfo &MRI,
   assert(SpvType && "VReg is expected to have SPIRV type");
   bool IsFloat = SpvType->getOpcode() == SPIRV::OpTypeFloat;
   bool IsVectorFloat =
       SpvType->getOpcode() == SPIRV::OpTypeVector &&
       GR.getSPIRVTypeForVReg(SpvType->getOperand(1).getReg())->getOpcode() ==
           SPIRV::OpTypeFloat;
   IsFloat |= IsVectorFloat;
   auto GetIdOp = IsFloat ? SPIRV::GET_fID : SPIRV::GET_ID;
-  auto DstClass = IsFloat ? &SPIRV::fIDRegClass : &SPIRV::IDRegClass;
   if (MRI.getType(ValReg).isPointer()) {
     NewT = LLT::pointer(0, 32);
     GetIdOp = SPIRV::GET_pID;
-    DstClass = &SPIRV::pIDRegClass;
   } else if (MRI.getType(ValReg).isVector()) {
     NewT = LLT::fixed_vector(2, NewT);
     GetIdOp = IsFloat ? SPIRV::GET_vfID : SPIRV::GET_vID;
-    DstClass = IsFloat ? &SPIRV::vfIDRegClass : &SPIRV::vIDRegClass;
   }
   Register IdReg = MRI.createGenericVirtualRegister(NewT);
-  MRI.setRegClass(IdReg, DstClass);
   return {IdReg, GetIdOp};
 }
 
 static void processInstr(MachineInstr &MI, MachineIRBuilder &MIB,
                          MachineRegisterInfo &MRI, SPIRVGlobalRegistry *GR) {
   unsigned Opc = MI.getOpcode();
   assert(MI.getNumDefs() > 0 && MRI.hasOneUse(MI.getOperand(0).getReg()));
   MachineInstr &AssignTypeInst =
       *(MRI.use_instr_begin(MI.getOperand(0).getReg()));
   auto NewReg = createNewIdReg(MI.getOperand(0).getReg(), Opc, MRI, *GR).first;
   AssignTypeInst.getOperand(1).setReg(NewReg);
   MI.getOperand(0).setReg(NewReg);
-  MIB.setInsertPt(*MI.getParent(),
-                  (MI.getNextNode() ? MI.getNextNode()->getIterator()
-                                    : MI.getParent()->end()));
+  MIB.setInsertPt(*MI.getParent(), MI.getIterator());
   for (auto &Op : MI.operands()) {
     if (!Op.isReg() || Op.isDef())
       continue;
     auto IdOpInfo = createNewIdReg(Op.getReg(), Opc, MRI, *GR);
-    MIB.buildInstr(IdOpInfo.second).addDef(IdOpInfo.first).addUse(Op.getReg());
+    auto B = MIB.buildInstr(IdOpInfo.second)
+                 .addDef(IdOpInfo.first)
+                 .addUse(Op.getReg());
     Op.setReg(IdOpInfo.first);
+
+    const auto &ST = GR->CurMF->getSubtarget();
+    constrainSelectedInstRegOperands(*B.getInstr(), *ST.getInstrInfo(),
+                                     *ST.getRegisterInfo(),
+                                     *ST.getRegBankInfo());
   }
 }
 
 // Defined in SPIRVLegalizerInfo.cpp.
 extern bool isTypeFoldingSupported(unsigned Opcode);
 
 static void processInstrsWithTypeFolding(MachineFunction &MF,
                                          SPIRVGlobalRegistry *GR,
for (MachineInstr &MI : MBB) {
       // as it operates on gMIR only.
       if (MI.getOpcode() != SPIRV::ASSIGN_TYPE)
         continue;
       Register SrcReg = MI.getOperand(1).getReg();
       unsigned Opcode = MRI.getVRegDef(SrcReg)->getOpcode();
       if (!isTypeFoldingSupported(Opcode))
         continue;
       Register DstReg = MI.getOperand(0).getReg();
-      if (MRI.getType(DstReg).isVector())
-        MRI.setRegClass(DstReg, &SPIRV::IDRegClass);
       // Don't need to reset type of register holding constant and used in
       // G_ADDRSPACE_CAST, since it braaks legalizer.
       if (Opcode == TargetOpcode::G_CONSTANT && MRI.hasOneUse(DstReg)) {
         MachineInstr &UseMI = *MRI.use_instr_begin(DstReg);
         if (UseMI.getOpcode() == TargetOpcode::G_ADDRSPACE_CAST)
           continue;
       }
       MRI.setType(DstReg, LLT::scalar(32));