[PATCH] D112410: [SPIR-V] Add a toolchain for SPIR-V in clang

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh accepted this revision.
svenvh added a comment.
This revision is now accepted and ready to land.

Mostly some minor comments that you can address at commit time.

It would be good to get approval from another reviewer.



================
Comment at: clang/docs/UsersManual.rst:3538
+<https://github.com/KhronosGroup/SPIRV-LLVM-Translator#build-instructions>`_
+for more details. Clang will expects the ``llvm-spirv`` executable to
+be present in the ``PATH`` environment variable. Clang uses ``llvm-spirv``
----------------



================
Comment at: clang/test/Driver/spirv-toolchain.cl:10
+// SPV64-SAME: "-o" [[BC:".*bc"]]
+// SPV64: {{".*llvm-spirv.*"}} [[BC]] "-o" {{".*o"}}
+
----------------
svenvh wrote:
> Anastasia wrote:
> > svenvh wrote:
> > > Any reason to not just check for `llvm-spirv{{.*}}`, for consistency with the clang check above?
> > Good question, apparently some tools get some target prefixes like if you look at `Driver::generatePrefixedToolNames`:
> > https://clang.llvm.org/doxygen/Driver_8cpp_source.html#l05169
> > 
> > But perhaps it doesn't happen for `llvm-spirv` and we can safely omit the prefix?
> Having a target prefix for llvm-spirv seems a bit redundant indeed.  `Driver::generatePrefixedToolNames` seems to add both a prefixed and non-prefixed tool name.
I see you have taken out the checking of the prefix; my suggestion was to write `llvm-spirv{{.*}}`, to align with the clang check above.


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

https://reviews.llvm.org/D112410