[PATCH] D115786: [SPIR-V 2/n] Add SPIRV{InstrFormats,InstrInfo,RegisterInfo,RegisterBanks...}.td

[Sven van Haastregt via Phabricator via llvm-commits]

svenvh added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRV.td:47
+}
\ No newline at end of file

----------------
Several files in this patch are missing their newline at EOF.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:88
+
+//3.32.1 Miscellaneous Instructions
+
----------------
The numbering doesn't correspond to the latest unified spec [1].  It is probably impractical to keep those numbers in sync as the spec keeps evolving.  But perhaps we can provide a spec version number to avoid confusion?

[1] https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_miscellaneous_instructions


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:151
+//3.32.6 Type-Declaration Instructions
+
+
----------------
Remove 1 newline here.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:240
+                      [(set ID:$dst, (assigntype ConstPseudoNull, TYPE:$src_ty))]>;
+//def OpConstantNull: Op<46, (outs ID:$res), (ins TYPE:$ty), "$res = OpConstantNull $ty">;
+
----------------
This can probably be removed?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:492-494
+//def OpShiftRightLogical: BinOp<"OpShiftRightLogical", 194>;
+//def OpShiftRightArithmetic: BinOp<"OpShiftRightArithmetic", 195>;
+//def OpShiftLeftLogical: BinOp<"OpShiftLeftLogical", 196>;
----------------
This can probably be removed?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:594
+let isTerminator=1 in {
+def OpBranch: Op<249, (outs), (ins ID:$label), "OpBranch $label">;
+def OpBranchConditional: Op<250, (outs), (ins ID:$cond, ID:$true, ID:$false, variable_ops),
----------------
Please indent.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:691-692
+
+// 3.32.22. Device-Side Enqueue Instructions,
+// 3.32.23. Pipe Instructions,
+
----------------
This patch doesn't add any instructions for these, so we could just as well remove these comments?  Or alternatively, if we plan to support them, mark as `TODO`.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstrInfo.td:742
+
+// and possibly 3.32.25. Reserved Instructions.
----------------
Not sure if this comment is relevant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115786