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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits