[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/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:
> 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.
Of course the series should be consistent but I think it doesn't make sense for us to strive for transitive closure at this stage, because it is only the first step towards adding (to LLVM) a full-featured SPIR-V backend, which will be useful for end users. Approaching this goal in accordance with the logic of the LLVM project is the meaning of the current work.

Probably it's worth adding more functionality to SPIRVGlobalTypesAndRegNumPass.cpp so more tests from our test base can initially pass. But as Matt commented we should make changes in the way SPIRVGlobalTypesAndRegNumPass works, I'm going to try it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116464