Skip to content

[cmake] Combine -fsycl-targets across all enabled GPU backends (fixes #708) - #753

Open
zjin-lcf wants to merge 1 commit into
uxlfoundation:developfrom
zjin-lcf:fix/708-multi-vendor-fsycl-targets
Open

[cmake] Combine -fsycl-targets across all enabled GPU backends (fixes #708)#753
zjin-lcf wants to merge 1 commit into
uxlfoundation:developfrom
zjin-lcf:fix/708-multi-vendor-fsycl-targets

Conversation

@zjin-lcf

Copy link
Copy Markdown
Contributor

Summary

Fixes #708.

example_lapack_getrs_usm (and other USM examples) throw a SYCL No kernel named ... was found exception on an NVIDIA GPU when oneMath is built with multiple GPU vendor backends enabled at once (e.g. CUDA + Intel + AMD).

Root cause

The SYCL offload targets were set on the shared ONEMATH::SYCL::SYCL interface target in two places:

  • cmake/FindCompiler.cmake used a mutually-exclusive if/elseif, so only one vendor's -fsycl-targets triple was ever selected.
  • Each vendor backend's CMakeLists.txt (cublas, rocblas, cusparse) additionally appended its own -fsycl-targets flag.

DPC++ only honors the last -fsycl-targets flag on the command line, so in a multi-vendor build all but the last triple were silently dropped. The device images for the other vendor(s) were never generated, so their SYCL kernels (e.g. the getrf 32→64-bit pivot-casting parallel_for in the cuSOLVER backend) were missing at runtime.

Fix

  • Assemble the offload target triples for all enabled GPU backends once, in cmake/FindCompiler.cmake, and pass them together as a single comma-separated -fsycl-targets=... list (with per-target -Xsycl-target-backend=<triple> arch options).
  • Preserve the Intel GPU spir64 image when ENABLE_MKLGPU_BACKEND is combined with a CUDA/HIP backend.
  • Remove the now-redundant per-backend -fsycl-targets appends from the cublas, rocblas, and cusparse CMakeLists.txt.

For a tri-vendor build this now produces e.g. -fsycl-targets=nvptx64-nvidia-cuda,amdgcn-amd-amdhsa,spir64, so every enabled vendor gets a device image. Single-vendor builds are unchanged.

Test plan

DPC++ only honors the last -fsycl-targets flag on the command line. oneMath
appended -fsycl-targets to the shared ONEMATH::SYCL::SYCL interface from
several vendor backends independently (FindCompiler.cmake, cublas, rocblas,
cusparse), so in a multi-vendor build all but the last target triple were
silently dropped. The device kernels of the other vendor(s) - notably the
32<->64-bit ipiv casting parallel_for in the LAPACK getrf/getrs USM wrappers,
which are the only real SYCL device kernels in those backends - were then
missing at runtime, producing "No kernel named ...getrf... was found"
(issue uxlfoundation#708).

Assemble a single comma-separated -fsycl-targets list covering every enabled
CUDA/ROCm backend (plus spir64 when the Intel oneMKL GPU backend is enabled)
in FindCompiler.cmake, with per-target -Xsycl-target-backend arch options, and
drop the now-redundant per-backend -fsycl-targets appends.

Fixes uxlfoundation#708

Co-authored-by: Cursor <cursoragent@cursor.com>
@melonakos

Copy link
Copy Markdown
Contributor

Heads up: I'm about to close and immediately reopen this PR. Nothing is wrong with it and nothing about your branch changes — that's just the cleanest way for a maintainer to trigger a fresh CI run.

Why it needs one: the red unit tests ArmPL * jobs here are not caused by your change. Arm republished their apt repo package as arm-toolchains-repository_2-2~noble_all.deb around 2026-07-30, which broke the 2-1 URL pinned in .github/workflows/pr-arm.yml. Your aarch64 run executed on 2026-08-11 while that pin was still stale, so Install ArmPL failed before any test ran. #749 fixed the pin on develop at 2026-08-11 23:21 UTC, a few hours later.

A plain re-run doesn't help, because re-running replays the original commit's workflow file — I tried, and it fetched the old 2-1 URL again. A fresh pull_request run picks up current develop and should go green.

@melonakos melonakos closed this Aug 26, 2026
@melonakos melonakos reopened this Aug 26, 2026

@melonakos melonakos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This is the right fix for #708 and it's a real architectural improvement over what was there — the per-backend CMake files were fighting each other over a flag that only one of them could win.

The diagnosis is correct and the mechanism matters

DPC++ only honors the last -fsycl-targets flag on the command line, so all target triples must be passed together as a single comma-separated list.

That's the bug. Each backend was doing target_compile_options(ONEMATH::SYCL::SYCL INTERFACE -fsycl-targets=...) on the same shared interface target, so in a cuBLAS + rocBLAS build one vendor's triple silently overwrote the other's and that vendor's device kernels simply weren't in the binary. Failing at runtime with missing kernels rather than at build time is exactly the sort of bug that costs someone a day, so this is worth having.

Three things in here I want to call out because they're easy to miss and you got them right:

-Xsycl-target-backend=<triple> instead of the bare form. The old code passed -Xsycl-target-backend --offload-arch=${HIP_TARGETS} with no triple. That's unambiguous only while there's exactly one target; the moment the list has two entries the arch option has to say which target it applies to. Switching to -Xsycl-target-backend=amdgcn-amd-amdhsa is required for this to work at all, not just tidier.

The old backend conditions were incomplete. The previous if tested ENABLE_CURAND_BACKEND OR ENABLE_CUSOLVER_BACKEND OR ENABLE_CUSPARSE_BACKEND — no cuBLAS, no cuFFT. So a cuBLAS-only build never entered that branch and depended entirely on the per-backend CMake file to supply the flag, which is how the whole thing ended up duplicated in three places. Adding ENABLE_CUBLAS_BACKEND, ENABLE_CUFFT_BACKEND and ENABLE_ROCFFT_BACKEND fixes a latent inconsistency as well as centralising.

Preserving spir64 for MKLGPU. This is the subtle one. Passing an explicit -fsycl-targets list replaces the default rather than adding to it, so an Intel GPU backend built alongside CUDA would quietly lose its own device image — the same class of bug as #708, just one layer down. Handling it explicitly, with a comment saying why, is good work.

I also checked the two behavioural edges: set_target_properties is now unconditional, but UNIX_INTERFACE_COMPILE_OPTIONS is seeded with -fsycl and only appended to when a GPU backend is on, so a CPU-only build still gets exactly -fsycl as before. And AdaptiveCpp is unaffected because FindCompiler.cmake is only pulled in on the dpc++ path, which matches the old per-backend if(NOT ... adaptivecpp) guards. Both equivalent, both simpler.

list(JOIN) needs CMake 3.12 and the project requires 3.13, so that's fine.

Merge order: this should land before #757

I raised this on #757 and reading this PR confirms it. #757 adds a -mcode-object-version=5 block inside the elseif(ENABLE_ROCBLAS_BACKEND OR ...) branch that this PR deletes, so there's a direct textual conflict.

More importantly the interaction is semantic. #757 is currently correct because the old if/elseif made the AMD branch unreachable whenever a CUDA backend was enabled — so its flag could never reach an NVIDIA compile. This PR removes that mutual exclusion by design. After it lands, a CUDA + ROCm build enters both blocks, and #757's flag would be appended to the shared UNIX_INTERFACE_COMPILE_OPTIONS and applied to the nvptx target too.

So: land this one first, then rebase #757 and re-scope its flag to the AMD target — most naturally as another -Xsycl-target-backend=amdgcn-amd-amdhsa entry in ONEMATH_SYCL_TARGET_ARCH_OPTIONS, which is exactly the hook this PR creates. That's a tidier result than #757 has today.

One asymmetry worth fixing while you're here

The CUDA arch options are guarded:

if(DEFINED CUDA_TARGETS AND NOT "${CUDA_TARGETS}" STREQUAL "")

but the HIP ones are added unconditionally:

list(APPEND ONEMATH_SYCL_TARGET_ARCH_OPTIONS
  -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=${HIP_TARGETS})

HIP_TARGETS is declared in the top-level CMakeLists.txt as set(HIP_TARGETS "" CACHE STRING ...) with no validation, so a user who enables a ROCm backend and forgets -DHIP_TARGETS=gfx90a gets --offload-arch= with nothing after it and a confusing compiler error. That's pre-existing rather than something you introduced, but you're the one consolidating this logic and the CUDA side already shows the pattern. Either mirror the guard, or better, emit a FATAL_ERROR naming the missing variable — an AMD build genuinely can't proceed without it, so failing early with a clear message beats failing later with an obscure one.

How did you verify it?

The scenario this fixes — two vendor backends enabled together — isn't built in CI, so nothing here can confirm the fix mechanically. Did you build a CUDA + ROCm configuration locally and check both device images are present? clang-offload-bundler --list on one of the resulting objects, or just confirming kernels run on both devices, would be worth a line in the PR description. Not asking you to re-run it for my benefit — but since CI can't cover this and the failure mode is silent, having the verification recorded somewhere means the next person touching this file knows what "working" looked like.

Comment thread cmake/FindCompiler.cmake
list(APPEND ONEMATH_SYCL_TARGET_TRIPLES "amdgcn-amd-amdhsa")
list(APPEND ONEMATH_SYCL_TARGET_ARCH_OPTIONS
-Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=${HIP_TARGETS})
endif()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melonakos mentioned this in his approval message, but I think it does need addressing. This PR changes rocFFT-only behavior by applying an unguarded HIP architecture option even though rocFFT previously did not require Xsycl-target-backend or --offload-arch flags. Should ENBALBE_ROCFFT_BACKEND be excluded from the if on lines 66-67? Alternatively, if it is kept then this should guard against an empty HIP_TARGETS as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

example_lapack_getrs_usm throws SYCL exception on NVIDIA GPU when built with support for AMD GPU backends

3 participants