[SYCL][Driver] Add option to specify the location of ocloc - #22912
[SYCL][Driver] Add option to specify the location of ocloc#22912mdtoguchi wants to merge 19 commits into
Conversation
Some new offload model tests within sycl-offload-jit.cpp will fail when the corresponding device libraries are not built. In those cases, the test needs to pick up the internal variants so the driver does not error due to not finding any.
The ocloc tool used for ahead of time compilation targeting Intel GPUs is acquired externally, so it is not guaranteed to be placed in the PATH. Add --ocloc-path=<dir>, allowing a user to specify the directory in which ocloc resides. The user provided location always wins over any ocloc that is otherwise visible via the program paths or the PATH environment variable. The option is honored for both the old and the new offloading model: - Old model: the driver's Intel GPU backend compile job (SYCL.cpp) and the ocloc help emitted for -fsycl-help=gen. - New model: forwarded to clang-linker-wrapper, and to clang-sycl-linker for the --sycl-link path. Both tools also accept --ocloc-path= directly for standalone use.
mdtoguchi
changed the title
tahonermann
left a comment
tahonermann
left a comment
There was a problem hiding this comment.
This looks ok to me. I'd like to see more comments explaining what happens if ocloc is not at the specified path or not found elsewhere.
It's unfortunate that the logic for finding the right path is repeated in so many places, but that is a pre-existing issue that probably isn't easy to fix.
The help text for each of the three cases of the new option differs for each, but not in a meaningful way as far as I can tell. I suggest consolidating to one phrasing.
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Does it make sense to submit it upstream first?
Probably not - the only tool where ocloc is even mentioned is the |
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Question: can we use -B instead of introducing new per-tool option?
Fixes a few items from review - Improved error handling with -fsycl-help=gen - Make getOclocPath a static helper - make --ocloc-path= usage consistent when no value provided
6e0734d to
fff04f4
Compare
Use of |
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
mostly lgtm, just some minor questions
There was a problem hiding this comment.
Pull request overview
Adds a new SYCL driver/user-facing option, --ocloc-path=<dir>, to let users specify where the externally-provided Intel GPU AOT tool ocloc resides, and wires it through both the old and new offloading flows (including -fsycl-help=gen tool help emission).
Changes:
- Introduces
--ocloc-path=<dir>as a clang driver option and forwards it toclang-linker-wrapperandclang-sycl-linkeras needed. - Implements lookup precedence so the user-provided directory is used ahead of program paths /
PATH, and rejects empty values early in the driver. - Adds/updates tests and documentation for the new option across driver, tools, and manuals.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| sycl/doc/UsersManual.md | Documents the new --ocloc-path user option. |
| clang/include/clang/Options/Options.td | Defines the new clang driver option. |
| clang/lib/Driver/Driver.cpp | Rejects empty --ocloc-path= and avoids running external tools on already-rejected command lines; uses shared ocloc resolution for help emission. |
| clang/lib/Driver/ToolChains/SYCL.h | Declares shared getOclocPath helper for ocloc resolution. |
| clang/lib/Driver/ToolChains/SYCL.cpp | Implements getOclocPath and uses it for old-model Intel GPU backend compilation. |
| clang/lib/Driver/ToolChains/Clang.cpp | Forwards --ocloc-path to clang-linker-wrapper under the new offload driver. |
| clang/lib/Driver/ToolChains/SPIRV.cpp | Forwards --ocloc-path to clang-sycl-linker for --sycl-link. |
| clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td | Adds --ocloc-path option to the linker-wrapper tool. |
| clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp | Implements ocloc lookup honoring --ocloc-path. |
| clang/tools/clang-sycl-linker/SYCLLinkOpts.td | Adds --ocloc-path option to the sycl-linker tool. |
| clang/tools/clang-sycl-linker/ClangSYCLLinker.cpp | Implements ocloc lookup honoring --ocloc-path. |
| clang/docs/ClangLinkerWrapper.rst | Documents --ocloc-path for clang-linker-wrapper. |
| clang/docs/ClangSYCLLinker.rst | Documents --ocloc-path for clang-sycl-linker. |
| clang/test/Driver/sycl-ocloc-path.cpp | New driver tests covering old/new model behavior, forwarding, diagnostics, and quoting. |
| clang/test/Driver/clang-linker-wrapper.cpp | Adds a linker-wrapper test for --ocloc-path. |
| clang/test/OffloadTools/clang-sycl-linker/basic.ll | Adds sycl-linker tests for --ocloc-path and diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tahonermann
left a comment
tahonermann
left a comment
There was a problem hiding this comment.
Changes look good to me; I don't have any further requests. Thanks @mdtoguchi!
|
@intel/dpcpp-clang-driver-reviewers, please take a look. |
The ocloc tool used for ahead of time compilation targeting Intel GPUs is
acquired externally, so it is not guaranteed to be placed in the PATH.
Add --ocloc-path=dir, allowing a user to specify the directory in which
ocloc resides. The user provided location always wins over any ocloc that
is otherwise visible via the program paths or the PATH environment
variable.
The option is honored for both the old and the new offloading model
the ocloc help emitted for -fsycl-help=gen.
for the --sycl-link path. Both tools also accept --ocloc-path=
directly for standalone use.