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

[Aleksandr Bezzubikov via Phabricator via llvm-commits]

zuban32 added inline comments.


================
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?
In SPIRV [almost] every value is an ID which is just an integer (usually 32- or 64-bit ), the type of the value with an ID is detemined by the instruction producing it which has 'ResultType' parameter. But we're trying not to enforce that rule on gMIR level so that before the selection all of the types were the ones IRTranslator emitted without anyhting target-specific interfering.


================
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?
Well, yes and no. In fact LLTs do not matter after we generated all the necessary type-assigning pseudo instructions. But LLTs still matter in sence of gMIR being well-formed (passing verification) and not having any significant differences from the original IR (i.e. scalars instead of vectors, vectors of different type, etc) The addrspace of LLT doesn't matter for sure


================
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
We placed this code here because it's doing some sort of legalization (in old SPIR-V versions pointers can't be compared directly, ptrtoints are required), not sure if it's good to mix things up in the selector.


================
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
The single-use constraint is enforced by the design of our target, every non-void instruction have the only user - type assignment instruction, which in turn may have multiple users just as the original instruction before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464