[OpenMP] Prevent AMDGPU from overriding visibility on DT_nohost variables

[jhuber6]

Summary:
There's some logic in the AMDGPU target that manually resets the
requested visibility of certain variables. This was triggering when we
set a constant variable in OpenMP. However, we shouldn't do this for
OpenMP when the variable has the nohost type. That implies that the
variable is not visible to the host and therefore does not need to be
visible, so we should respect the original value of it.

[llvmbot]

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-clang

Changes

Summary:
There's some logic in the AMDGPU target that manually resets the
requested visibility of certain variables. This was triggering when we
set a constant variable in OpenMP. However, we shouldn't do this for
OpenMP when the variable has the nohost type. That implies that the
variable is not visible to the host and therefore does not need to be
visible, so we should respect the original value of it.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/Targets/AMDGPU.cpp (+4)
• (modified) clang/test/OpenMP/target_visibility.cpp (+5-1)

diff --git a/clang/lib/CodeGen/Targets/AMDGPU.cpp b/clang/lib/CodeGen/Targets/AMDGPU.cpp
index dc628b7345f59fd..70f906a42e6d9c4 100644
--- a/clang/lib/CodeGen/Targets/AMDGPU.cpp
+++ b/clang/lib/CodeGen/Targets/AMDGPU.cpp
@@ -308,6 +308,10 @@ static bool requiresAMDGPUProtectedVisibility(const Decl *D,
   if (GV->getVisibility() != llvm::GlobalValue::HiddenVisibility)
     return false;
 
+  if (auto *Attr = D->getAttr<OMPDeclareTargetDeclAttr>())
+    if (OMPDeclareTargetDeclAttr::DT_NoHost == Attr->getDevType())
+      return false;
+
   return D->hasAttr<OpenCLKernelAttr>() ||
          (isa<FunctionDecl>(D) && D->hasAttr<CUDAGlobalAttr>()) ||
          (isa<VarDecl>(D) &&
diff --git a/clang/test/OpenMP/target_visibility.cpp b/clang/test/OpenMP/target_visibility.cpp
index 938d164df89bffb..2ed90d4b8ba9a77 100644
--- a/clang/test/OpenMP/target_visibility.cpp
+++ b/clang/test/OpenMP/target_visibility.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -debug-info-kind=limited -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-target-device -o - | FileCheck %s
-// RUN: %clang_cc1 -debug-info-kind=limited -verify -fopenmp -x c++ -triple nvptx-unknown-unknown -fopenmp-targets=nvptx-nvidia-cuda -emit-llvm %s -fopenmp-is-target-device -o - | FileCheck %s
+// RUN: %clang_cc1 -debug-info-kind=limited -verify -fopenmp -x c++ -triple amdgcn-amd-amdhsa -fopenmp-targets=amdgcn-amd-amdhsa -emit-llvm %s -fopenmp-is-target-device -o - | FileCheck %s
 // expected-no-diagnostics
 
 
@@ -21,6 +21,10 @@ void B::bar() { A a; a.foo(); }
 void B::sbar() { A::sfoo(); }
 #pragma omp declare target to(B::bar, B::sbar)
 
+[[gnu::visibility("hidden")]] extern const int x = 0;
+#pragma omp declare target to(x) device_type(nohost)
+
+// CHECK-DAG: @x = hidden{{.*}} constant i32 0
 // CHECK-DAG: define hidden void @_ZN1B4sbarEv()
 // CHECK-DAG: define linkonce_odr hidden void @_ZN1A4sfooEv()
 // CHECK-DAG: define hidden void @_ZN1B3barEv(

[yxsamliu]

maybe it is safer to have tests for both amdgcn and nvptx since they behave differently

[jhuber6]

I just added --check-prefixes=CHECK,AMDGPU for the differences. Figured it's not worth making a new test just for this.

[JonChesterfield]

This stuff looks very cuda/opencl specific. It's definitely surprising for C++ code. Do we need it for openmp? If not it seems better to guard the hack with visibility behind if (hip)

[jhuber6]

This stuff looks very cuda/opencl specific. It's definitely surprising for C++ code. Do we need it for openmp? If not it seems better to guard the hack with visibility behind if (hip)

You reminded me that I need to refine this logic as well. What it's doing here is basically the same as what we want to happen. If someone uses #pragma omp declare target on a variable without device_type(nohost) that means the host runtime will need to register it. If it's hidden then that will be a lookup error. I refined the logic to do the same handling for OpenMP, but ignore it if it's DT_nohost because then there's no registration code required.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JonChesterfield]

is this a spurious whitespace change?

[jhuber6]

Not here, above there's an extra line. I'll fix that before I land, not worth force pushing.

[jdoerfert]

Seems reasonable to me.

[jdoerfert]

LG if others don't object