[PATCH] D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov 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) {
----------------
arsenm wrote:
> Since this returns only literals, return StringRef?
Ok, it will be applied to other expanded macros that also return literals.


================
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
----------------
arsenm wrote:
> This line wrapping is unreadable, plus I don't trust all of these unchecked casts. Does the verifier enforce these rules for the metadata?
I'm not sure that it is checked by verifier.
I'm sorry, this code fragment is not needed right now (since the series of patches does not include the pass that generates this metadata). I'll exclude it from this patch. We'll verify and correct the fragment by the next time.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:205
+  auto M = MIRBuilder.getMF().getFunction().getParent();
+  Function *Callee = M->getFunction(FuncName);
+  Register FuncVReg;
----------------
arsenm wrote:
> 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
Thanks, it will be fixed.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:204-207
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
+  MIRBuilder.setMBB(MBB);
+  MIRBuilder.setInstr(I);
----------------
arsenm wrote:
> You shouldn't have to construct a fresh new MIRBuilder for every selected instruction
As I understand you suggest constructing MachineIRBuilder closer to its users, because not all instructions (for selection) are actually use it.


================
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)
----------------
arsenm wrote:
> 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
Yes, I'll fix it.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:177
+
+  getActionDefinitionsBuilder(G_PHI).alwaysLegal();
+
----------------
arsenm wrote:
> I thought SPIRV had a fixed set of register types?
@arsenm yes, all register types are legal for phis. This rule will be substituted by legalFor(allPtrsScalarsAndVectors).


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:286
+
+static std::pair<Register, unsigned>
+createNewIdReg(Register ValReg, unsigned Opcode, MachineRegisterInfo &MRI,
----------------
arsenm wrote:
> I'm not sure what's meant by an "Id Reg"
I suppose it means a register which is used in GET_ID pseudo instruction.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:301
+  if (MRI.getType(ValReg).isPointer()) {
+    NewT = LLT::pointer(0, 32);
+    GetIdOp = SPIRV::GET_pID;
----------------
arsenm wrote:
> Are you intentionally discarding the pointer size and address space?
It's used in creation of new temporary pseudos which are involved in the instruction selection and then are removed. Perhaps it's a bad practice and should be avoided.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:310
+  auto IdReg = MRI.createGenericVirtualRegister(NewT);
+  MRI.setRegClass(IdReg, DstClass);
+  return {IdReg, GetIdOp};
----------------
arsenm wrote:
> Generally code in the legalizer shouldn't bother setting regbanks or classes
Probably it would be better to move this to selector.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:332
+      auto *SpirvType = GR->getOrCreateSPIRVType(LLVMTy, Helper.MIRBuilder);
+      GR->assignSPIRVTypeToVReg(SpirvType, ResVReg, Helper.MIRBuilder);
+    }
----------------
arsenm wrote:
> You're doing a lot of work to convert to the SPIRV types here, which would be more appropriate to do during selection
@arsenm Yes, we'll try to move all this work to the selection stage. 
@zuban32 I have verified that we don't need this code here (see Khronos repo).


================
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);
----------------
arsenm wrote:
> it might be cleaner to push this cast to integer into the generic LegalizerHelper
Ok, I have created a function that generates a single cast.


================
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);
----------------
arsenm wrote:
> You're assuming there's only a single use, and your legalize action should only touch the passed instruction
Instructions processed here should always have a single use which is a pseudo instruction ASSIGN_TYPE, generated by SPIRV specific pass before the legalizer. We'll add a check that this condition is met. Also it looks like the modification of another instructions may be avoided. 
And this fragment works together with another pass that is not presented in this series, so I would postpone it for now. 


================
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();
----------------
arsenm wrote:
> 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
Ok, we'll get rid of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116464/new/

https://reviews.llvm.org/D116464