[SYCL][Driver] Add option to specify the location of ocloc by mdtoguchi · Pull Request #22912 · intel/llvm

mdtoguchi · 2026-08-10T19:42:45Z

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.

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.

…river2

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.

tahonermann

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.

tahonermann · 2026-08-11T22:45:00Z

+// Returns the full path of the ocloc tool to be used for AOT compilation.  A
+// user provided --ocloc-path= is honored above all other lookup locations.
+const char *getOclocPath(Compilation &C, const ToolChain &TC,
+                         const llvm::opt::ArgList &Args);


What happens when ocloc is not found? Is a null pointer returned? Is a diagnostic issued? It would be helpful for the comment to state what should happen.

If not found, the fallback is just the tool name with no path information. I'll update the comment.

YuriPlyakhin

Does it make sense to submit it upstream first?

mdtoguchi · 2026-08-12T22:35:51Z

Does it make sense to submit it upstream first?

Probably not - the only tool where ocloc is even mentioned is the clang-sycl-linker. All other locations, no AOT behavior is available. This probably makes more sense to upstream when more AOT functionality is available.

YuriPlyakhin · 2026-08-12T23:06:21Z

 }

+/// Locate the 'ocloc' tool used for Intel GPU AOT compilation.
+Expected<std::string> findOcloc(const ArgList &Args) {


Implementation in ClangLinkerWrapper.cpp and ClangSYCLLinker.cpp - near-identical, but differ in some subtle things:

The two copies check DryRun on opposite sides of the findProgramByName call. In the wrapper, a --dry-run in an environment where ocloc does exist in the given directory hits the filesystem and returns the resolved path; in clang-sycl-linker it never does.

Diagnostic capitalization is different (Unable to find vs unable to find).

Could you please align implementations?

aligned here: d27f11d

YuriPlyakhin

Question: can we use -B instead of introducing new per-tool option?

YuriPlyakhin · 2026-08-12T23:25:24Z

+    SmallString<128> ExecPath;
+    // A user provided --ocloc-path= overrides the usual tool lookup for ocloc.
+    if (Arg *A = C.getArgs().getLastArg(options::OPT_ocloc_path_EQ);
+        A && std::get<1>(HA) == "ocloc") {


nit
std::get<1>(HA) is now repeated three times in five lines - a bit difficult to read the code.
Could you please consider a small named struct ({Triple, ToolName, HelpFlag, ExtraArg}) instead of tuple (std::tuple<llvm::Triple, StringRef, StringRef, StringRef>)? Maybe you can do a small NFC PR with this refactoring as a prerequisite to this PR.

I'll look into restructuring this for readability. Current changes at least clean up the std::get<1>(HA) usage.

YuriPlyakhin · 2026-08-12T23:28:51Z

-        C.getDefaultToolChain().GetProgramPath(std::get<1>(HA).data()));
+    SmallString<128> ExecPath;
+    // A user provided --ocloc-path= overrides the usual tool lookup for ocloc.
+    if (Arg *A = C.getArgs().getLastArg(options::OPT_ocloc_path_EQ);


getLastArg(OPT_ocloc_path_EQ) is re-queried on every loop iteration for every tool, please, consider hoisting it above the loop or maybe change the order of conditions, to first check if the tool is ocloc.

should no longer be an issue with the change to use a common ocloc location query.

YuriPlyakhin · 2026-08-12T23:34:39Z

 def cuda_path_EQ : Joined<["--"], "cuda-path=">,
  Flags<[WrapperOnlyOption]>, MetaVarName<"<dir>">,
  HelpText<"Set the system CUDA path">;
+def ocloc_path_EQ : Joined<["--"], "ocloc-path=">,


Please, update documentation for both: clang-linker-wrapper and clang-sycl-linker to document new options (also driver maybe?)
ClangLinkerWrapper.rst
ClangSYCLLinker.rst
sycl/doc/UsersManual.md

Also Options WG???

Documentation additions here: cb367d5

Options WG can be queried, but is currently more geared towards xmain and new options that require additional scrutiny. If this new option naming is somewhat controversial, a proposal can be submitted.

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

mdtoguchi · 2026-08-14T00:04:36Z

Question: can we use -B instead of introducing new per-tool option?

Use of -B<dir> already works (when using the old model), but it is a general application and impacts all GetProgramPath lookups. There is already precedence of using a specific option for the specific tool to locate.

sarnex

mostly lgtm, just some minor questions

sarnex · 2026-08-14T14:54:41Z

+    // Forward the user provided location for ocloc.
+    if (Arg *A = Args.getLastArg(options::OPT_ocloc_path_EQ))
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("--ocloc-path=") + A->getValue()));


can we have a test where the path contains spaces just to make sure we don't get any nonsense

add here: dc8860f

sarnex · 2026-08-14T15:13:39Z

+    // An empty --ocloc-path= is rejected here for consistent usage for areas
+    // that consume it.
+    if (A->getOption().matches(options::OPT_ocloc_path_EQ) &&
+        A->containsValue("")) {


will this fail if the path contains a space?

It's only checking for no argument and doesn't care if the string itself is malformed or contains a space.

ah sorry i misread the code thanks

sarnex · 2026-08-14T15:17:20Z

    }
    // Run the Tool.
-    llvm::sys::ExecuteAndWait(ToolBinary.get(), ToolArgs);
+    if (llvm::sys::ExecuteAndWait(ToolBinary.get(), ToolArgs) < 0)


probably a nit:
the doc for ExecuteAndWait says

Returns an integer result code indicating the status of the program. A zero or positive value indicates the result code of the program. -1 indicates failure to execute -2 indicates a crash during execution or timeout

So it will definitely return something < 0 if it failed to run/crash, but I think it will be positive if it ran but ocloc returns non-zero. Do we want to throw the error in that case or is that case handled elsewhere?

I will update to cover all non-zero. The call here is just for emitting tool help information

fixed here: 1a47a4b

sarnex

thanks!

sarnex · 2026-08-14T18:56:06Z

    }
    // Run the Tool.
-    llvm::sys::ExecuteAndWait(ToolBinary.get(), ToolArgs);
+    if (!llvm::sys::ExecuteAndWait(ToolBinary.get(), ToolArgs))


i might be overthinking this but we only want to go into the if if the result is not exacltly 0 right? either < 0 or >0 we want to error right?, if so i dont think this condition does that

Hmm... I think my brain is fried.

sarnex

thx

mdtoguchi added 5 commits July 24, 2026 11:37

Merge remote-tracking branch 'intel_llvm/sycl' into san-test-update-d…

da28bd4

…river2

Merge remote-tracking branch 'intel_llvm/sycl' into ocloc-location

0e0a736

Update func name

d3b9cfe

mdtoguchi changed the title ~~Ocloc location~~ [SYCL][Driver] Add option to specify the location of ocloc Aug 10, 2026

Improve windows testing

a469b82

mdtoguchi marked this pull request as ready for review August 11, 2026 01:11

mdtoguchi requested review from a team as code owners August 11, 2026 01:11

tahonermann requested changes Aug 11, 2026

View reviewed changes

mdtoguchi added 3 commits August 12, 2026 14:36

Sync up comments and help text

b4c1da6

Fix quotes

7a70b18

extraneous character

03c8be4

YuriPlyakhin reviewed Aug 12, 2026

View reviewed changes

Comment thread clang/lib/Driver/Driver.cpp Outdated

YuriPlyakhin reviewed Aug 12, 2026

View reviewed changes

Comment thread clang/lib/Driver/ToolChains/SYCL.h Outdated

YuriPlyakhin reviewed Aug 12, 2026

View reviewed changes

Comment thread clang/lib/Driver/ToolChains/SYCL.cpp

YuriPlyakhin reviewed Aug 12, 2026

View reviewed changes

Update --ocloc-path= usage behaviors

fff04f4

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

mdtoguchi force-pushed the ocloc-location branch from 6e0734d to fff04f4 Compare August 13, 2026 21:11

mdtoguchi added 3 commits August 13, 2026 15:20

Use common func for grabbing ocloc location

5e0620a

Sync findOcloc behaviors in tools

d27f11d

Add appropriate documentation for the new option

cb367d5

mdtoguchi requested a review from a team as a code owner August 14, 2026 00:08

sarnex reviewed Aug 14, 2026

View reviewed changes

Add tests for directories with spaces

dc8860f

sarnex approved these changes Aug 14, 2026

View reviewed changes

Error for non-zero

1a47a4b

mdtoguchi requested review from YuriPlyakhin and tahonermann August 14, 2026 18:41

sarnex reviewed Aug 14, 2026

View reviewed changes

Fix condition

ec6e84d

sarnex approved these changes Aug 14, 2026

View reviewed changes

Uh oh!

Conversation

mdtoguchi commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdtoguchi commented Aug 12, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdtoguchi commented Aug 14, 2026

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

mdtoguchi commented Aug 10, 2026 •

edited

Loading

sarnex Aug 14, 2026 •

edited

Loading