[cmake] Combine -fsycl-targets across all enabled GPU backends (fixes #708) - #753
[cmake] Combine -fsycl-targets across all enabled GPU backends (fixes #708)#753zjin-lcf wants to merge 1 commit into
Conversation
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>
|
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 A plain re-run doesn't help, because re-running replays the original commit's workflow file — I tried, and it fetched the old |
melonakos
left a comment
There was a problem hiding this comment.
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-targetsflag 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.
| 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() |
There was a problem hiding this comment.
@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.
Summary
Fixes #708.
example_lapack_getrs_usm(and other USM examples) throw a SYCLNo kernel named ... was foundexception 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::SYCLinterface target in two places:cmake/FindCompiler.cmakeused a mutually-exclusiveif/elseif, so only one vendor's-fsycl-targetstriple was ever selected.CMakeLists.txt(cublas,rocblas,cusparse) additionally appended its own-fsycl-targetsflag.DPC++ only honors the last
-fsycl-targetsflag 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. thegetrf32→64-bit pivot-castingparallel_forin the cuSOLVER backend) were missing at runtime.Fix
cmake/FindCompiler.cmake, and pass them together as a single comma-separated-fsycl-targets=...list (with per-target-Xsycl-target-backend=<triple>arch options).spir64image whenENABLE_MKLGPU_BACKENDis combined with a CUDA/HIP backend.-fsycl-targetsappends from thecublas,rocblas, andcusparseCMakeLists.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
-DENABLE_MKLGPU_BACKEND+ allCU*+ allROC*backends (-DHIP_TARGETS=gfx1100) as in example_lapack_getrs_usm throws SYCL exception on NVIDIA GPU when built with support for AMD GPU backends #708.example_lapack_getrs_usmon the NVIDIA GPU and confirm it no longer throwsNo kernel named ... was found.