[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: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);
----------------
iliya-diyachkov wrote:
> zuban32 wrote:
> > zuban32 wrote:
> > > arsenm wrote:
> > > > zuban32 wrote:
> > > > > iliya-diyachkov wrote:
> > > > > > 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. 
> > > > > 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
> > > Why not if we have a target-specific LLVM IR pass making that assumption always true?
> > Oh, I see. It's just the pass responsible for making the input IR compliant with what SPIR-V specific GlobalISel stages expect is currently missing from these patches. Maybe we need to extend the series
> As Renato suggested in the comment (see the first patch), we need a minimum set of patches that does something substantial. Then we'll add passes/features by stand alone patches extending the SPIR-V backend functionality and making more tests passed. I'm not sure that right now we have a reason to increase the series noticeably.
>  
Yep, I understand that. The question is whether initial series should be kind of transitive closure or there might be some logical inconsistency due to some parts of the code not being upstreamed yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464