[PATCH] D116465: [SPIRV 6/6] Add 2 essential passes and the simplest tests

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBlockLabeler.cpp:102
+
+// Add OpLabels and terminators. Fix any instructions with MBB references
+bool SPIRVBlockLabeler::runOnMachineFunction(MachineFunction &MF) {
----------------
arsenm wrote:
> Why does this need to be a codegen pass? Can't you handle this during the encoding/emission?
Each OpLabel defines a virtual register which is used in terminator instructions. I'm not sure that creating OpLabels, then finding and patching correspondent terminators is an appropriate task for the encoding/emission. Moreover the next pass (SPIRVGlobalTypesAndRegNumPass) should rename all virtual registers globally (i.e. each register in a module gets an unique number). As a result OpLabels and terminators (since they have registers) should be processed before SPIRVGlobalTypesAndRegNumPass.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBlockLabeler.cpp:104
+bool SPIRVBlockLabeler::runOnMachineFunction(MachineFunction &MF) {
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
----------------
arsenm wrote:
> Using MachineIRBuilder outside of a globalisel pass is weird
Yes, we need to use BuildMI like in addPreEmitPass2's passes of other targets.


================
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:
> > 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.


================
Comment at: llvm/test/CodeGen/SPIRV/metadata-opencl12.ll:1
+; RUN: llc -O0 -global-isel %s -o - | FileCheck %s
+
----------------
arsenm wrote:
> Probably should make global-isel the target default so you don't have to add the flag to every test
Yes, I agree.


================
Comment at: llvm/test/CodeGen/SPIRV/optnone.ll:6
+; FunctionControlDontInlineMask = 0x2 (2)
+; CHECK-SPIRV-NO-EXT: %{{[0-9]+}} = OpFunction %{{[0-9]+}} DontInline
+
----------------
MaskRay wrote:
> Without a positive test in the same file, a negative pattern can easily go stale without being noticed.
> 
> Consider `split-file` to place highly related tests together.
Actually this is a positive test with the confusing checker name CHECK-SPIRV-NO-EXT. It was copied form SPIRV Translator test base as many of our LIT tests (sometimes with simplification). I suppose it is not a good candidate for this series, so I'll find another one instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116465