[PATCH] D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 4 11:05:34 PST 2022
arsenm added inline comments.
================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:28
+// Dim must be implemented manually, as "1D" is not a valid C++ naming token
+std::string getDimName(Dim::Dim dim) {
+ switch (dim) {
----------------
Since this returns only literals, return StringRef?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:115
+
+ // this code restores function args/retvalue types
+ // for composite cases because the final types should still be aggregate
----------------
Capitalize
================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:118
+ // whereas they're i32 during the translation to cope with
+ // aggregates flattenning etc
+ auto *NamedMD = F.getParent()->getNamedMetadata("spv.cloned_funcs");
----------------
s/aggregates flattenning/aggregate flattening/
================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:130-140
+ auto *ThisFuncMD = *ThisFuncMDIt;
+ if (cast<ConstantInt>(
+ cast<ConstantAsMetadata>(
+ cast<MDNode>(ThisFuncMD->getOperand(1))->getOperand(0))
+ ->getValue())
+ // TODO: currently -1 indicates return value, support this types
+ // renaming for arguments as well
----------------
This line wrapping is unreadable, plus I don't trust all of these unchecked casts. Does the verifier enforce these rules for the metadata?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:205
+ auto M = MIRBuilder.getMF().getFunction().getParent();
+ Function *Callee = M->getFunction(FuncName);
+ Register FuncVReg;
----------------
You should absolutely not be checking the module or relying on the callee being a named function. You should only be inspecting the provided CallLoweringInfo for callee information. This will fail on indirect calls or if the callee is a constexpr bitcast
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:83-84
+ auto EleOpc = ElemType->getOpcode();
+ if (EleOpc != OpTypeInt && EleOpc != OpTypeFloat && EleOpc != OpTypeBool)
+ report_fatal_error("Invalid vector element type");
+
----------------
This should probably be an assert, not report_fatal_error
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:101-102
+ if (SpvType) {
+ Type *LLVMTy = const_cast<Type *>(getTypeForSPIRVType(SpvType));
+ assert(LLVMTy->isIntegerTy());
+ LLVMIntTy = cast<IntegerType>(LLVMTy);
----------------
use cast<IntegerType>?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:128-129
+ if (SpvType) {
+ Type *LLVMTy = const_cast<Type *>(getTypeForSPIRVType(SpvType));
+ assert(LLVMTy->isFloatingPointTy());
+ LLVMFPTy = LLVMTy;
----------------
Same as above, avoid const_cast and use normal cast mechanism for type assertions
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:214-215
+ bool EmitIR) {
+ if (ElemType->getOpcode() == SPIRV::OpTypeVoid)
+ report_fatal_error("Invalid array element type");
+ Register NumElementsVReg =
----------------
should be assert?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:242
+ .addUse(getSPIRVTypeID(RetType));
+ for (const auto &ArgType : ArgTypes)
+ MIB.addUse(getSPIRVTypeID(ArgType));
----------------
don't use the reference and just use the pointer values?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:259
+ return getOpTypeVoid(MIRBuilder);
+ else if (Ty->isVectorTy()) {
+ auto El = getOrCreateSPIRVType(cast<FixedVectorType>(Ty)->getElementType(),
----------------
No return after else
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:267-268
+ return getOpTypeArray(Ty->getArrayNumElements(), El, MIRBuilder, EmitIR);
+ } else if (dyn_cast<StructType>(Ty)) {
+ llvm_unreachable("Unsupported LLVM type");
+ } else if (auto FType = dyn_cast<FunctionType>(Ty)) {
----------------
assert / isa
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:276
+ return getOpTypeFunction(RetTy, ParamTypes, MIRBuilder);
+ } else if (const auto &PType = dyn_cast<PointerType>(Ty)) {
+ Type *ElemType = PType->getPointerElementType();
----------------
No const reference
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:281-282
+ // but should be treated as custom types like OpTypeImage
+ if (dyn_cast<StructType>(ElemType))
+ llvm_unreachable("Unsupported LLVM type");
+
----------------
assert(!isa<StructType>())
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:326
+ return true;
+ } else if (Type->getOpcode() == SPIRV::OpTypeVector) {
+ Register ScalarTypeVReg = Type->getOperand(1).getReg();
----------------
No return after else
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:342
+ Type->getOpcode() == SPIRV::OpTypeFloat)) {
+ return Type->getOperand(1).getImm();
+ } else if (Type && Type->getOpcode() == SPIRV::OpTypeBool) {
----------------
No return after else
================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h:9-12
+// SPIRVGlobalRegistry is used to maintain rich type information required for
+// SPIR-V even after lowering from LLVM IR to GMIR. It can convert an llvm::Type
+// into an OpTypeXXX instruction, and map it to a virtual register. Also it
+// builds and supports consistency of constants and global variables.
----------------
Why do you need to do this? What are you losing in gMIR that matters?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:204-207
+ MachineIRBuilder MIRBuilder;
+ MIRBuilder.setMF(MF);
+ MIRBuilder.setMBB(MBB);
+ MIRBuilder.setInstr(I);
----------------
You shouldn't have to construct a fresh new MIRBuilder for every selected instruction
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:518
+ assert(I.hasOneMemOperand());
+ auto MemOp = *I.memoperands_begin();
+ auto Scope = getScope(MemOp->getSyncScopeID());
----------------
MaskRay wrote:
> The file may have too many `auto`. See https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
I agree there are too many autos
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:592-609
+ if (!WithSuccess) // If we just need the old Val, not {oldVal, Success}
+ return Success;
+ assert(ResType->getOpcode() == SPIRV::OpTypeStruct);
+ auto BoolReg = MRI->createGenericVirtualRegister(LLT::scalar(1));
+ Register BoolTyID = ResType->getOperand(2).getReg();
+ Success &= MIRBuilder.buildInstr(SPIRV::OpIEqual)
+ .addDef(BoolReg)
----------------
I think you are just repeating the generic expansion for this. It would be better to just use the default lower action for G_ATOMIC_CMPXCHG_WITH_SUCCESS
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:595
+ assert(ResType->getOpcode() == SPIRV::OpTypeStruct);
+ auto BoolReg = MRI->createGenericVirtualRegister(LLT::scalar(1));
+ Register BoolTyID = ResType->getOperand(2).getReg();
----------------
LLT is shorter and clearer than auto
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:640
+ return selectUnOp(ResVReg, ResType, I, MIRBuilder, OpPtrCastToGeneric);
+ } else if (SrcSC == SC::Generic && isGenericCastablePtr(DstSC)) {
+ // We're casting from Generic to an eligable pointer
----------------
No return after else
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:660
+ } else {
+ // TODO Should this case just be disallowed completely?
+ // We're casting 2 other arbitrary address spaces, so have to bitcast
----------------
return after else
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:727-728
+ default:
+ report_fatal_error("Unknown predicate type for ICmp: " +
+ CmpInst::getPredicateName(Pred));
+ }
----------------
This should just be an llvm_unreachable
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:753-754
+ default:
+ report_fatal_error("Unknown predicate type for Bool comparison: " +
+ CmpInst::getPredicateName(Pred));
+ }
----------------
This should just be an llvm_unreachable
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:810-813
+ if (Cmp0Type->getOpcode() != OpTypePointer &&
+ (!GR.isScalarOrVectorOfType(Cmp0, scalarTyOpc) ||
+ !GR.isScalarOrVectorOfType(Cmp1, scalarTyOpc)))
+ llvm_unreachable("Incompatible type for comparison");
----------------
Shouldn't check this, this should be redundant with the verifier/legalizer rule checks
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:835-843
+ if (STI.canDirectlyComparePointers()) {
+ std::tie(CmpOpc, PtrToUInt) = getPtrCmpOpcode(Pred);
+ } else {
+ PtrToUInt = true;
+ }
+ } else if (GR.isScalarOrVectorOfType(CmpOperand, SPIRV::OpTypeBool)) {
+ TypeOpc = SPIRV::OpTypeBool;
----------------
It looks like you're repeating work done in the legalizer?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:922
+ return OneVec;
+ } else {
+ return OneReg;
----------------
No else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:973
+ return selectSelect(ResVReg, ResType, I, IsSigned, MIRBuilder);
+ } else {
+ return selectUnOp(ResVReg, ResType, I, MIRBuilder,
----------------
No else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1012
+ return selectIntToBool(IntReg, ResVReg, ArgType, ResType, MIRBuilder);
+ } else {
+ bool IsSigned = GR.isScalarOrVectorSigned(ResType);
----------------
No else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1029
+ .constrainAllUses(TII, TRI, RBI);
+ else {
+ auto MIB = MIRBuilder.buildInstr(SPIRV::OpConstantI)
----------------
Else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1101
+ return true;
+ } else {
+ // Must be relying on implicit block fallthrough, so generate an
----------------
No else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1138
+ auto GlobalIdent = GV->getGlobalIdentifier();
+ auto GlobalVar = dyn_cast<GlobalVariable>(GV);
+
----------------
Should just use cast<> and remove the check, this should never fail. I believe we disallow null G_GLOBAL_VALUE sources
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:177
+
+ getActionDefinitionsBuilder(G_PHI).alwaysLegal();
+
----------------
I thought SPIRV had a fixed set of register types?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:202-203
+ if (retTy.isVector())
+ return cmpTy.isVector() &&
+ retTy.getNumElements() == cmpTy.getNumElements();
+ else
----------------
This is illegal MIR, there's no point in checking it
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:204
+ retTy.getNumElements() == cmpTy.getNumElements();
+ else
+ // ST.canDirectlyComparePointers() for ponter args is
----------------
Else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:205
+ else
+ // ST.canDirectlyComparePointers() for ponter args is
+ // checked in legalizeCustom().
----------------
Typo ponter
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:217-218
+ if (retTy.isVector()) {
+ return cmpTy.isVector() &&
+ retTy.getNumElements() == cmpTy.getNumElements();
+ } else {
----------------
You don't need to worry about the cases that are illegal MIR
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:219
+ retTy.getNumElements() == cmpTy.getNumElements();
+ } else {
+ return cmpTy.isScalar();
----------------
Else after return
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:233
+
+ // Struct return types become a single large scalar, so cannot easily legalize
+ getActionDefinitionsBuilder({G_ATOMIC_CMPXCHG, G_ATOMIC_CMPXCHG_WITH_SUCCESS})
----------------
This has been not true for about 2 years
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:286
+
+static std::pair<Register, unsigned>
+createNewIdReg(Register ValReg, unsigned Opcode, MachineRegisterInfo &MRI,
----------------
I'm not sure what's meant by an "Id Reg"
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:301
+ if (MRI.getType(ValReg).isPointer()) {
+ NewT = LLT::pointer(0, 32);
+ GetIdOp = SPIRV::GET_pID;
----------------
Are you intentionally discarding the pointer size and address space?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:310
+ auto IdReg = MRI.createGenericVirtualRegister(NewT);
+ MRI.setRegClass(IdReg, DstClass);
+ return {IdReg, GetIdOp};
----------------
Generally code in the legalizer shouldn't bother setting regbanks or classes
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:332
+ auto *SpirvType = GR->getOrCreateSPIRVType(LLVMTy, Helper.MIRBuilder);
+ GR->assignSPIRVTypeToVReg(SpirvType, ResVReg, Helper.MIRBuilder);
+ }
----------------
You're doing a lot of work to convert to the SPIRV types here, which would be more appropriate to do during selection
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:348-355
+ Helper.MIRBuilder.buildInstr(TargetOpcode::G_PTRTOINT)
+ .addDef(ConvReg0)
+ .addUse(Op0.getReg());
+ Helper.MIRBuilder.buildInstr(TargetOpcode::G_PTRTOINT)
+ .addDef(ConvReg1)
+ .addUse(Op1.getReg());
+ Op0.setReg(ConvReg0);
----------------
it might be cleaner to push this cast to integer into the generic LegalizerHelper
================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:361-365
+ 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);
----------------
You're assuming there's only a single use, and your legalize action should only touch the passed instruction
================
Comment at: llvm/lib/Target/SPIRV/SPIRVUtils.cpp:132-133
+ return StorageClass::Workgroup;
+ case 4:
+ return StorageClass::Generic;
+ case 7:
----------------
How/why are you not using generic as addrspace 0?
================
Comment at: llvm/lib/Target/SPIRV/SPIRVUtils.cpp:160-169
+bool constrainRegOperands(MachineInstrBuilder &MIB, MachineFunction *MF) {
+ if (!MF)
+ MF = MIB->getMF();
+ const auto &Subtarget = MF->getSubtarget();
+ const TargetInstrInfo *TII = Subtarget.getInstrInfo();
+ const TargetRegisterInfo *TRI = Subtarget.getRegisterInfo();
+ const RegisterBankInfo *RBI = Subtarget.getRegBankInfo();
----------------
I dislike this wrapper function. You should have all of these variables available in the context where you are constraining already, and the MF parameter with null check is potentially confusing
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116464/new/
https://reviews.llvm.org/D116464
More information about the llvm-commits
mailing list