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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added a comment.

I think all the CHECKs we have in the added tests do check instruction Printer (we don't use Parser) since all the lines in asm output (OpName, OpType***, OpFunction, OpLabel, OpFunctionParameter etc) are instructions in terms of SPIR-V and Printer prints them as normal instructions. As you can see, some of them have arguments that are also checked for correctness.

The actual reason for adding the only simplest tests is not the missed frame lowering. SPIR-V uses high-level type information that should be maintained after IR translator and other GlobalISel passes. The necessary passes and utility code have not been added yet, since the simplest tests can be passed without them. However, if we want to have a test that contains something substantial, these functionality (>1k lines) should be added. But I'm not sure we really need that in this series of patches.

By the way, slightly extending the second pass I can add a test that has a simple argument usage with value returning.

The 2 passes in this patch are actually checked by the added tests because the tests check the instructions added during the passes (e.g. OpLabel/OpFunctionEnd in the first pass and OpSource/OpName in the second one).



================
Comment at: llvm/lib/Target/SPIRV/SPIRVBlockLabeler.cpp:11
+// start with an OpLabel, and ends with a suitable terminator (OpBranch is
+// inserted if necessary).
+//
----------------
rengolin wrote:
> Isn't this guaranteed by IR passes and the verifier?
> 
> What would create basic blocks that don't have a suitable terminator that still passes the verify?
It looks like an outdated description. In any case, I can't immediately find a test that would require additional terminators at the time of the pass invocation.

The main purpose of the pass is to insert OpLabel instructions and to put the corresponding registers to OpBranchConditional, OpBranch, and OpPhi instructions. Also OpFunctionEnd (last instruction in functions) is inserted here.

These instructions are required to pass simple tests with defined functions.


================
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.
+//
----------------
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.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp:158
+// Add custom passes right before emitting asm/obj files.
+void SPIRVPassConfig::addPreEmitPass2() {
+  // Insert missing block labels and terminators. Fix instrs with MBB references
----------------
rengolin wrote:
> I don't see where this is called in this code...
It's overrided standard LLVM pass which is called immediately before machine code is emitted (llvm/include/llvm/CodeGen/TargetPassConfig.h:443).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116465