[PATCH] D144494: [SPIR-V] Support TargetExtType for SPIR-V builtin types

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added a comment.

> I think converting most of the tests to be based on SPIR-V builtin types (2) and only keeping a handful of tests using pointers-to-opaque-struct (1) to demonstrate compatibility with old IR makes most sense.

I agree.

Thanks for the patch. Overall, it looks good to me. Minor suggestions are noted below.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp:1974
+  // parameters should have the following format:
+  // e.g. %spirv.Event
+  if (!NameWithParameters.startswith("spirv."))
----------------
Maybe use the same style as in previous comment (line 1961):
```
  // Pointers-to-opaque-structs representing OpenCL types are first translated
  // to equivalent SPIR-V types. OpenCL builtin type names should have the
  // following format: e.g. %opencl.event_t
```




================
Comment at: llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp:1975-1976
+  // e.g. %spirv.Event
+  if (!NameWithParameters.startswith("spirv."))
+    llvm_unreachable("Unknown builtin opaque type!");
+
----------------
It could be assert().


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp:2095
   // "Lower" the BuiltinType into TargetType. The following get<...>Type methods
-  // use the implementation details from TableGen records to either create a new
-  // OpType<...> machine instruction or get an existing equivalent SPIRVType
-  // from GlobalRegistry.
+  // use the implementation details from TableGen records  or TargetExtType
+  // parameters to either create a new OpType<...> machine instruction or get an
----------------
Double space in "records  or".


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

https://reviews.llvm.org/D144494