[OpenMP] Remove hidden visibility for declare target variables

[jhuber6]

Summary

This patch changes the visiblity of variables declared within a declare
target directive. Variable declarations within a declare target
directive need to be externally visible to the plugin for initialization
or reading. Previously this would cause runtime errors where the named
global could not be found because it was not included in the symbol
table.

[jhuber6]

Changing method to use an early exit, previously visibility would still be hidden for some constructs.

[jhuber6]

Updating again, wasn't handling static variables declared in a record context
properly.

[jdoerfert]

LG, let's hope this solves all the AMDGPU issues we've seen.

[JonChesterfield]

If I'm following correctly, this broke the amdgpu buildbot and it has been moved into staging as a workaround. I haven't debugged what breaks but 'DefaultVisibility' is relatively likely to behave differently on cuda vs hsa systems so that's my first guess.

clang/lib/AST/Decl.cpp

792

Would this change static variables to non-static and thus introduce multiple definition errors? Not immediately obvious to me that the variables have to be directly visible to other translation units

[jhuber6]

In D117362#3254681, @JonChesterfield wrote:

If I'm following correctly, this broke the amdgpu buildbot and it has been moved into staging as a workaround. I haven't debugged what breaks but 'DefaultVisibility' is relatively likely to behave differently on cuda vs hsa systems so that's my first guess.

Yes, this patch is necessary because of the different handling of hidden visibility by NVIDIA and AMDGPU. Without this patch we cannot run any code that uses #pragma omp declare target on AMDGPU because they are all hidden. The runtime will try to load it and fail so we will crash.

clang/lib/AST/Decl.cpp

792

That was my first guess, I had @ronlieb run an alternate approach where we run everything as normal, but at the end we inherit the linkage but set the visibility to default. he said it still caused problems.

[JonChesterfield]

clang/lib/AST/Decl.cpp

1080

Possibly address space dependent. I'm not confident shared or thread local variables will behave sensibly if annotated with elf visibility flags.

clang/lib/AST/Decl.cpp

if (const auto *Var = dyn_cast<VarDecl>(D)) {
     //   given variable or function shall be identical...
     // C does not have an equivalent rule.
     //
     // Ignore this if we've got an explicit attribute;  the user
     // probably knows what they're doing.
     //
     // Note that we don't want to make the variable non-external
     // because of this, but unique-external linkage suits us.
+
+    // We need variables inside OpenMP declare target directives to be visible.
+    if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(Var))
+      return LinkageInfo::external();
+
     if (Context.getLangOpts().CPlusPlus && !isFirstInExternCContext(Var) &&
         !IgnoreVarTypeLinkage) {
       LinkageInfo TypeLV = getLVForType(*Var->getType(), computation);
       if (!isExternallyVisible(TypeLV.getLinkage()))
         return LinkageInfo::uniqueExternal();
       if (!LV.isVisibilityExplicit())
         LV.mergeVisibility(TypeLV);
     }
if (explicitSpecSuppressor &&
       LV.isVisibilityExplicit() &&
       classLV.getVisibility() != DefaultVisibility &&
       hasDirectVisibilityAttribute(explicitSpecSuppressor, computation)) {
     considerClassVisibility = false;
   }
 
   // Finally, merge in information from the class.
   LV.mergeMaybeWithVisibility(classLV, considerClassVisibility);
+
+  // We need variables inside OpenMP declare target directives to be visible.
+  if (const VarDecl *VD = dyn_cast<VarDecl>(D))
+    if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(VD))
+      return LinkageInfo(LV.getLinkage(), DefaultVisibility, false);
+
   return LV;
 }
 
 void NamedDecl::anchor() {}
 
 bool NamedDecl::isLinkageValid() const {
   if (!hasCachedLinkage())
     return true;

clang/test/OpenMP/declare_target_codegen.cpp

 // expected-no-diagnostics
 
 // SIMD-ONLY-NOT: {{__kmpc|__tgt}}
 // KMPC-ONLY-NOT: __tgt
 
 // CHECK-NOT: define {{.*}}{{baz1|baz4|maini1|Base|virtual_}}
 // CHECK-DAG: Bake
 // CHECK-NOT: @{{hhh|ggg|fff|eee}} =
-// CHECK-DAG: @flag = hidden global i8 undef,
+// CHECK-DAG: @flag = global i8 undef,
 // CHECK-DAG: @aaa = external global i32,
-// CHECK-DAG: @bbb ={{ hidden | }}global i32 0,
+// CHECK-DAG: @bbb = global i32 0,
 // CHECK-DAG: weak constant %struct.__tgt_offload_entry { i8* bitcast (i32* @bbb to i8*),
 // CHECK-DAG: @ccc = external global i32,
-// CHECK-DAG: @ddd ={{ hidden | }}global i32 0,
+// CHECK-DAG: @ddd = global i32 0,
 // CHECK-DAG: @hhh_decl_tgt_ref_ptr = weak global i32* null
 // CHECK-DAG: @ggg_decl_tgt_ref_ptr = weak global i32* null
 // CHECK-DAG: @fff_decl_tgt_ref_ptr = weak global i32* null
 // CHECK-DAG: @eee_decl_tgt_ref_ptr = weak global i32* null
 // CHECK-DAG: @{{.*}}maini1{{.*}}aaa = internal global i64 23,
 // CHECK-DAG: @pair = {{.*}}addrspace(3) global %struct.PAIR undef
-// CHECK-DAG: @b ={{ hidden | }}global i32 15,
-// CHECK-DAG: @d ={{ hidden | }}global i32 0,
+// CHECK-DAG: @_ZN2SS3SSSE = global i32 1,
+// CHECK-DAG: @b = global i32 15,
+// CHECK-DAG: @d = global i32 0,
 // CHECK-DAG: @c = external global i32,
-// CHECK-DAG: @globals ={{ hidden | }}global %struct.S zeroinitializer,
+// CHECK-DAG: @globals = global %struct.S zeroinitializer,
 // CHECK-DAG: [[STAT:@.+stat]] = internal global %struct.S zeroinitializer,
 // CHECK-DAG: [[STAT_REF:@.+]] = internal constant %struct.S* [[STAT]]
-// CHECK-DAG: @out_decl_target ={{ hidden | }}global i32 0,
+// CHECK-DAG: @out_decl_target = global i32 0,
 // CHECK-DAG: @llvm.used = appending global [2 x i8*] [i8* bitcast (void ()* @__omp_offloading__{{.+}}_globals_l[[@LINE+84]]_ctor to i8*), i8* bitcast (void ()* @__omp_offloading__{{.+}}_stat_l[[@LINE+85]]_ctor to i8*)],
 // CHECK-DAG: @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (%struct.S** [[STAT_REF]] to i8*)],
 
 // CHECK-DAG: define {{.*}}i32 @{{.*}}{{foo|bar|baz2|baz3|FA|f_method}}{{.*}}()
 // CHECK-DAG: define {{.*}}void @{{.*}}TemplateClass{{.*}}(%class.TemplateClass* {{[^,]*}} %{{.*}})
 // CHECK-DAG: define {{.*}}i32 @{{.*}}TemplateClass{{.*}}f_method{{.*}}(%class.TemplateClass* {{[^,]*}} %{{.*}})
 // CHECK-DAG: define {{.*}}void @__omp_offloading__{{.*}}_globals_l[[@LINE+78]]_ctor()
 

 
 #ifdef LOAD
 #pragma omp declare target
 void new_bar1() {
   TTT<int> *X = new TTT<int>();
   X->emitted();
 }
 #pragma omp end declare target
+
+struct SS {
+#pragma omp declare target
+  static int SSS;
+#pragma omp end declare target
+};
+int SS::SSS = 1;
 #endif

clang/test/OpenMP/declare_target_only_one_side_compilation.cpp

 int GX;
 static int GY;
 #pragma omp end declare variant
 #pragma omp end declare target
 #endif
 
 // TODO: It is odd, probably wrong, that we don't mangle all variables.
 
-// DEVICE-DAG: @G1 = hidden {{.*}}global i32 0, align 4
+// DEVICE-DAG: @G1 = {{.*}}global i32 0, align 4
 // DEVICE-DAG: @_ZL2G2 = internal {{.*}}global i32 0, align 4
-// DEVICE-DAG: @G3 = hidden {{.*}}global i32 0, align 4
+// DEVICE-DAG: @G3 = {{.*}}global i32 0, align 4
 // DEVICE-DAG: @_ZL2G4 = internal {{.*}}global i32 0, align 4
-// DEVICE-DAG: @G5 = hidden {{.*}}global i32 0, align 4
+// DEVICE-DAG: @G5 = {{.*}}global i32 0, align 4
 // DEVICE-DAG: @_ZL2G6 = internal {{.*}}global i32 0, align 4
 // DEVICE-NOT: ref
 // DEVICE-NOT: llvm.used
 // DEVICE-NOT: omp_offload
 
 // HOST-DAG: @G7 = global i32 0, align 4
 // HOST-DAG: @_ZL2G8 = internal global i32 0, align 4
 // HOST-DAG: @G9 = global i32 0, align 4
 // HOST-DAG: @_ZL3G10 = internal global i32 0, align 4
 // HOST-DAG: @G11 = global i32 0, align 4
 // HOST-DAG: @_ZL3G12 = internal global i32 0, align 4