[PATCH] D116465: [SPIRV 6/6] Add one essential pass and the simplest tests

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalTypesAndRegNumPass.cpp:23
+// that are technically invalid as a result. As such, it must be the last pass,
+// and requires instruction verification to be disabled afterwards.
+//
----------------
arsenm wrote:
> iliya-diyachkov wrote:
> > arsenm wrote:
> > > iliya-diyachkov wrote:
> > > > rengolin wrote:
> > > > > That's a bit scary. Is that what created BBs without terminators?
> > > > Actually the pass creates a dummy function that represents the global context of the module containing types, constants, global variables  and other SPIR-V specific global information. This dummy function has a linear CFG, and in fact its BBs do not contain terminators, however, they can be inserted or a single BB can be used here (because all these BBs are created just for the convenience of the code motion from real functions to this dummy one).
> > > >  
> > > > This pass works just before the code emission and does not break any other passes. It is expected that the verifier will not work after it, since registers are renamed according to requirements for SPIR-V code emission, and it will be wrong for the verifier.
> > > I think having the verifier fail at any point is a problem. Why does this have to be a dedicated pass, and not just part of the AsmPrinter?
> > Currently only a part of this pass is presented in the review. The entire pass works at module level and performs following things: 
> > - create a dummy function with some instructions;
> > - move some other instructions from regular functions to that dummy function;
> > - rename all registers globally, each new register name is unique in a whole module.
> >  
> > I don't think that operating at global level (copying instructions from one function to another and global naming of registers) can be implemented using the AsmPrinter (please correct me if I'm wrong). However we can try to move some functionality to AsmPrinter and make MIR consistent after this pass, which solves the verifier issue.
> This sounds like a function of emission to me. I don't see why you need a dummy MIR function or move anything for the purpose of having a global value. You could just emit the global values directly where they need to go as part of the emission of the individual operation.
> 
> The AsmPrinter is a module pass, so it's possible to operate globally
Basically the dummy function feels like an awkward hack of SPIR-V's constraints into MIR. It would be more natural to handle the structural differences in representation when actually producing the final SPIR-V


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

https://reviews.llvm.org/D116465