[SYCL] Move driver related __CUDA_ARCH__ test to Driver folder from Preprocessor
#15521
Conversation
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
| @@ -0,0 +1,3 @@ | |||
| // RUN: %clangxx %s -fsycl -nocudalib -fno-sycl-libspirv -fsycl-targets=nvptx64-nvidia-cuda -Xsycl-target-backend --offload-arch=sm_80 -E -dM \ | |||
| // RUN: | FileCheck --check-prefix=CHECK-CUDA-ARCH-MACRO %s | |||
| // CHECK-CUDA-ARCH-MACRO-NOT: #define __CUDA_ARCH__ {{[0-9]+}} | |||
There was a problem hiding this comment.
Given the original nature of the driver change, one thing we should also verify is that -fcuda-is-device is not supplied with -fsycl -fsycl-targets=nvptx64-nvidia-cuda for the device compilation (-### check)
There was a problem hiding this comment.
@mdtoguchi Thanks for being rigorous here! That's definitely what the original change prevented, where the unwanted macro was a product of this. I've added the test and updated the PR description accordingly.
Let me know if you think something needs to be updated.
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| @@ -0,0 +1,11 @@ | |||
| // Verify the __CUDA_ARCH__ macro has not been defined when offloading SYCL on NVPTX | |||
There was a problem hiding this comment.
Are AMDGPU targets covered by other tests? FYI: #15544
There was a problem hiding this comment.
Thanks for catching this. I had noticed the macro being defined in the HIP toolchain at the time of fixing the regression for the Cuda flow. It isn't tested correctly.
I made a draft PR (here #15443) for HIP-AMDGPU right after the Cuda one and I was planning to make it ready for review after this gets merged. Seems others caught it too in the meantime. I'll add a sycl clang driver for AMDGPU targets here as part of the said PR.
|
@intel/dpcpp-cfe-reviewers, @intel/dpcpp-clang-driver-reviewers, ping. |
Since this is mentioned in the description, should this be tested as well? |
This is just so we can compile and run the tests without requiring an NVPTX target builds or having to specify manually the path to the libspirv libs. There isn't a behavioural change related to this to be tested. Maybe I can reword the description so it's less confusing as the changes here are just an update to a fix to the CUDA_ARCH macro from a previous PR. |
This PR moves the driver invocation test that checks
__CUDA_ARCH__does not get defined and ensures that it doesn't require thelibspirv-nvptx64-nvidia-cudabitcode files by passing-fno-sycl-libspirvto the%clangxxcommand.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-deviceoption is not supplied in the CC1 invocation targetingnvptx64-nvidia-cuda, which enablesLangOptions.CudaIsDeviceand was the cause of defining the__CUDA_ARCH__macro.