Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][Driver] Fix regression that enabled Cuda-mode in cc1 and defined __CUDA_ARCH__ #15441

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

GeorgeWeb
Copy link
Contributor

@GeorgeWeb GeorgeWeb commented Sep 19, 2024

The CudaToolChain set -fcuda-is-device unconditionally which made InitializePredefinedMacros (called from clang::InitializePreprocessor) to define __CUDA_ARCH__ (default-init to 1). As such, the driver assumed Cuda mode while in also SYCL mode, but we don't properly support Cuda device-code compatibility and we want to avoid having the __CUDA_ARCH__ macro defined altogether for SYCL offload.

@GeorgeWeb GeorgeWeb force-pushed the georgi/cuda-arch-define branch from 186d6bd to a188a0b Compare September 19, 2024 11:30
@GeorgeWeb GeorgeWeb changed the title Fix regression that enabled Cuda-mode speific cc1 args [SYCL][Driver] Fix regression that enabled Cuda-mode in cc1 and defined __CUDA_ARCH__ Sep 19, 2024
@GeorgeWeb GeorgeWeb marked this pull request as ready for review September 19, 2024 11:30
@GeorgeWeb GeorgeWeb requested review from a team as code owners September 19, 2024 11:30
@GeorgeWeb
Copy link
Contributor Author

@intel/llvm-gatekeepers This looks good to merge now. Thanks.

@sarnex sarnex merged commit 9fd767d into intel:sycl Sep 23, 2024
12 checks passed
// RUN: %clang_cc1 %s -triple nvptx64-nvidia-cuda -target-cpu sm_80 -fsycl-is-device -E -dM | FileCheck --check-prefix=CHECK-CUDA %s
// RUN: %clang_cc1 %s -triple nvptx64-nvidia-cuda -target-cpu sm_80 -fsycl-is-device -E -dM | FileCheck \
// RUN: --check-prefix=CHECK-CUDA %s -DARCH_CODE=800
// RUN: %clangxx %s -fsycl -nocudalib -fsycl-targets=nvptx64-nvidia-cuda -Xsycl-target-backend --offload-arch=sm_80 -E -dM | FileCheck \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the preprocessor test, so we should only call %clang_cc1. Please, move the driver test to clang/test/Driver/ directory.

While you are doing this, please, make sure you either have libspirv input files or pass -fno-sycl-libspirv option. I don't build NVPTX target and this test fails with:

clang: error: cannot find 'remangled-l64-signed_char.libspirv-nvptx64-nvidia-cuda.bc'; provide path to libspirv library via '-fsycl-libspirv-path', or pass '-fno-sycl-libspirv' to build without linking with libspirv
clang: error: cannot find 'remangled-l64-signed_char.libspirv-nvptx64-nvidia-cuda.bc'; provide path to libspirv library via '-fsycl-libspirv-path', or pass '-fno-sycl-libspirv' to build without linking with libspirv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I'll sort that out asap once I am back on a work PC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #15521

bader pushed a commit that referenced this pull request Oct 2, 2024
… Preprocessor (#15521)

This PR moves the driver invocation test that checks `__CUDA_ARCH__`
does not get defined and ensures that it doesn't require the
`libspirv-nvptx64-nvidia-cuda` bitcode files by passing
`-fno-sycl-libspirv` to the `%clangxx` command.
Link to the comment in related PR that reported this issue:
#15441 (comment)
Additionally, an extra test is added to check that the
`-fcuda-is-device` option is not supplied in the CC1 invocation
targeting `nvptx64-nvidia-cuda`, which enables
`LangOptions.CudaIsDevice` and was the cause of defining the
`__CUDA_ARCH__` macro.
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.

7 participants