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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp:177
+
+  getActionDefinitionsBuilder(G_PHI).alwaysLegal();
+
----------------
zuban32 wrote:
> 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.
That's the whole point of the legalizer. There's some notion of fixed supported types implied by the other legalizer rules


================
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);
----------------
zuban32 wrote:
> 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.
This is not a property the target can decide, the incoming IR can have multiple uses. It's simply wrong to rely on it like this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464