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

[AMDGPU][OpenMP] Do not attach -fcuda-is-device flag for AMDGPU OpenMP #96909

Merged

Conversation

DominikAdamski
Copy link
Contributor

-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and it does not need to be added as clang cc1 option for OpenMP code.

`-fcuda-is-device` flag is not used for OpenMP offloading for AMD GPUs
and it does not need to be added as clang cc1 option for OpenMP code.
@DominikAdamski DominikAdamski requested a review from jhuber6 June 27, 2024 13:53
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AMDGPU clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:openmp OpenMP related changes to Clang labels Jun 27, 2024
@DominikAdamski DominikAdamski requested a review from ronlieb June 27, 2024 13:53
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-amdgpu

Author: Dominik Adamski (DominikAdamski)

Changes

-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and it does not need to be added as clang cc1 option for OpenMP code.


Full diff: https://github.com/llvm/llvm-project/pull/96909.diff

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp (-2)
  • (modified) clang/test/Driver/amdgpu-openmp-toolchain.c (+1-1)
diff --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
index d17ecb15c8208..c7fef090cb625 100644
--- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -47,8 +47,6 @@ void AMDGPUOpenMPToolChain::addClangTargetOptions(
   assert(DeviceOffloadingKind == Action::OFK_OpenMP &&
          "Only OpenMP offloading kinds are supported.");
 
-  CC1Args.push_back("-fcuda-is-device");
-
   if (DriverArgs.hasArg(options::OPT_nogpulib))
     return;
 
diff --git a/clang/test/Driver/amdgpu-openmp-toolchain.c b/clang/test/Driver/amdgpu-openmp-toolchain.c
index 49af04acc4639..a153c4afb0ce8 100644
--- a/clang/test/Driver/amdgpu-openmp-toolchain.c
+++ b/clang/test/Driver/amdgpu-openmp-toolchain.c
@@ -7,7 +7,7 @@
 
 // verify the tools invocations
 // CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-llvm-bc"{{.*}}"-x" "c"
-// CHECK: "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu"{{.*}}"-fcuda-is-device"{{.*}}"-target-cpu" "gfx906"
+// CHECK: "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu"{{.*}}"-target-cpu" "gfx906"
 // CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-obj"
 // CHECK: clang-linker-wrapper{{.*}} "-o" "a.out"
 

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-clang-driver

Author: Dominik Adamski (DominikAdamski)

Changes

-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and it does not need to be added as clang cc1 option for OpenMP code.


Full diff: https://github.com/llvm/llvm-project/pull/96909.diff

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp (-2)
  • (modified) clang/test/Driver/amdgpu-openmp-toolchain.c (+1-1)
diff --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
index d17ecb15c8208..c7fef090cb625 100644
--- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -47,8 +47,6 @@ void AMDGPUOpenMPToolChain::addClangTargetOptions(
   assert(DeviceOffloadingKind == Action::OFK_OpenMP &&
          "Only OpenMP offloading kinds are supported.");
 
-  CC1Args.push_back("-fcuda-is-device");
-
   if (DriverArgs.hasArg(options::OPT_nogpulib))
     return;
 
diff --git a/clang/test/Driver/amdgpu-openmp-toolchain.c b/clang/test/Driver/amdgpu-openmp-toolchain.c
index 49af04acc4639..a153c4afb0ce8 100644
--- a/clang/test/Driver/amdgpu-openmp-toolchain.c
+++ b/clang/test/Driver/amdgpu-openmp-toolchain.c
@@ -7,7 +7,7 @@
 
 // verify the tools invocations
 // CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-llvm-bc"{{.*}}"-x" "c"
-// CHECK: "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu"{{.*}}"-fcuda-is-device"{{.*}}"-target-cpu" "gfx906"
+// CHECK: "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu"{{.*}}"-target-cpu" "gfx906"
 // CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-obj"
 // CHECK: clang-linker-wrapper{{.*}} "-o" "a.out"
 

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

We don't even pass this in the NVPTX offloading case, so there's no reason to do it for AMDGPU.

@DominikAdamski DominikAdamski merged commit 8bb00cb into llvm:main Jul 1, 2024
12 checks passed
DominikAdamski added a commit to DominikAdamski/llvm-project-upstream that referenced this pull request Jul 1, 2024
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 1, 2024
revert: breaks smoke virtfunc1 and virtual_functions
* 8bb00cb [AMDGPU][OpenMP] Do not attach -fcuda-is-device flag for AMDGPU OpenMP (llvm#96909)

Change-Id: Iddfaa6e382f0078303256e5e24a3a69af0d524d7
skatrak added a commit to ROCm/llvm-project that referenced this pull request Jul 2, 2024
@ajarmusch
Copy link
Contributor

This change seems to have broken a CI:

https://gitlab.e4s.io/uo-public/llvm-openmp-offloading/-/jobs/283716

Could you please take a look and revert if you need time to investigate?

DominikAdamski added a commit to DominikAdamski/llvm-project-upstream that referenced this pull request Jul 3, 2024
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
llvm#96909)

`-fcuda-is-device` flag is not used for OpenMP offloading for AMD GPUs
and it does not need to be added as clang cc1 option for OpenMP code.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
llvm#96909)

`-fcuda-is-device` flag is not used for OpenMP offloading for AMD GPUs
and it does not need to be added as clang cc1 option for OpenMP code.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
DominikAdamski added a commit that referenced this pull request Jul 18, 2024
-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and
it does not need to be added as clang cc1 option for OpenMP code.

This PR has the same functionality as
#96909 but it doesn't introduce
regression for virtual function support.
DominikAdamski added a commit to DominikAdamski/llvm-project-upstream that referenced this pull request Jul 18, 2024
This reverts commit f2e362e.

PR: llvm#99002 causes
that -fcuda-is-device is not added for AMD GPU OpenMP.
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Jul 22, 2024
-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and
it does not need to be added as clang cc1 option for OpenMP code.

This PR has the same functionality as
llvm#96909 but it doesn't introduce
regression for virtual function support.
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and
it does not need to be added as clang cc1 option for OpenMP code.

This PR has the same functionality as
llvm#96909 but it doesn't introduce
regression for virtual function support.
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
-fcuda-is-device flag is not used for OpenMP offloading for AMD GPUs and
it does not need to be added as clang cc1 option for OpenMP code.

This PR has the same functionality as
#96909 but it doesn't introduce
regression for virtual function support.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants