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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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) {
----------------
Why does this need to be a codegen pass? Can't you handle this during the encoding/emission?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBlockLabeler.cpp:104
+bool SPIRVBlockLabeler::runOnMachineFunction(MachineFunction &MF) {
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
----------------
Using MachineIRBuilder outside of a globalisel pass is weird


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


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalTypesAndRegNumPass.cpp:232
+    } else {
+      errs() << MI << "\n";
+      llvm_unreachable("Unexpected operand type when copying spirv meta instr");
----------------
MI print includes the newline already


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116465