[OpenMP] Change default visibility to protected for device declarations

[jhuber6]

Summary

This patch changes the special-case handling of visibility when
compiling for an OpenMP target offloading device. This was orignally
added as a precaution against the bug encountered in PR41826 when
symbols in the device were being preempted by shared library symbols.
This should instead be done by making the visibility protected by default.
With protected visibility we are asserting that the symbols on the device
will never be preempted or preempt another symbol pending a shared library
load.

[jhuber6]

Changing to use default protected instead of passing -Bsymbolic this should be more portable and make the intentions clearer.

[JonChesterfield]

Looks great, thanks!

[jdoerfert]

Does this still work with AMDGPU as we need it to?

clang/test/OpenMP/declare_target_codegen.cpp

293

What happened here?

[jhuber6]

clang/test/OpenMP/declare_target_codegen.cpp

293

That was a special case I added that was only necessary when we were trying to remove things from being hidden. Now that hidden is not the default it's not necessary anymore.

[jdoerfert]

clang/test/OpenMP/declare_target_codegen.cpp

293

can we keep the test (coverage) anyway

[jhuber6]

Changing to use '-fvisibility=protected' when we construct the job. This is much more transparent and leaves the option open for the user to override it if they need default visibility.

[JonChesterfield]

clang/lib/Driver/ToolChains/Clang.cpp

5830 ↗

(On Diff #401766)

I think we need some more code here to detect if the user has already specified a value for fvisibility so that we don't clobber it

[jhuber6]

Forgot to make this mutually exclusive with user defined visibility value.

[jhuber6]

clang/lib/Driver/ToolChains/Clang.cpp

5830 ↗

(On Diff #401766)

I put the code there with the intent of putting the else there, but forgot to actually put it there.

[JonChesterfield]

LG, thanks!

clang/lib/AST/Decl.cpp

if (const auto *Var = dyn_cast<VarDecl>(D)) {
     // 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);
     }
// Everything not covered here has no linkage.
     return LinkageInfo::none();
   }
 
   // If we ended up with non-externally-visible linkage, visibility should
   // always be default.
   if (!isExternallyVisible(LV.getLinkage()))
     return LinkageInfo(LV.getLinkage(), DefaultVisibility, false);
 
-  // Mark the symbols as hidden when compiling for the device.
+  // Mark the symbols as protected when compiling for the device.
   if (Context.getLangOpts().OpenMP && Context.getLangOpts().OpenMPIsDevice)
-    LV.mergeVisibility(HiddenVisibility, /*newExplicit=*/false);
+    LV.mergeVisibility(ProtectedVisibility, /*newExplicit=*/false);
 
   return LV;
 }
 
 LinkageInfo
 LinkageComputer::getLVForClassMember(const NamedDecl *D,
                                      LVComputationKind computation,
                                      bool IgnoreVarTypeLinkage) {
if (explicitSpecSuppressor &&
       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 = global i8 undef,
+// CHECK-DAG: @flag = protected global i8 undef,
 // CHECK-DAG: @aaa = external global i32,
-// CHECK-DAG: @bbb = global i32 0,
+// CHECK-DAG: @bbb ={{ protected | }}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 = global i32 0,
+// CHECK-DAG: @ddd ={{ protected | }}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: @_ZN2SS3SSSE = global i32 1,
-// CHECK-DAG: @b = global i32 15,
-// CHECK-DAG: @d = global i32 0,
+// CHECK-DAG: @b ={{ protected | }}global i32 15,
+// CHECK-DAG: @d ={{ protected | }}global i32 0,
 // CHECK-DAG: @c = external global i32,
-// CHECK-DAG: @globals = global %struct.S zeroinitializer,
+// CHECK-DAG: @globals ={{ protected | }}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 = global i32 0,
+// CHECK-DAG: @out_decl_target ={{ protected | }}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/nvptx_declare_target_var_ctor_dtor_codegen.cpp

 // RUN: %clang_cc1 -fopenmp-simd -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -include-pch %t -o - | FileCheck %s --check-prefix SIMD-ONLY
 
 #ifndef HEADER
 #define HEADER
 
 // SIMD-ONLY-NOT: {{__kmpc|__tgt}}
 
 // DEVICE-DAG: [[C_ADDR:.+]] = internal global i32 0,
-// DEVICE-DAG: [[CD_ADDR:@.+]] ={{ hidden | }}global %struct.S zeroinitializer,
+// DEVICE-DAG: [[CD_ADDR:@.+]] ={{ protected | }}global %struct.S zeroinitializer,
 // HOST-DAG: @[[C_ADDR:.+]] = internal global i32 0,
-// HOST-DAG: @[[CD_ADDR:.+]] ={{( hidden | dso_local)?}} global %struct.S zeroinitializer,
+// HOST-DAG: @[[CD_ADDR:.+]] ={{( protected | dso_local)?}} global %struct.S zeroinitializer,
 
 #pragma omp declare target
 int foo() { return 0; }
 #pragma omp end declare target
 int bar() { return 0; }
 #pragma omp declare target (bar)
 int baz() { return 0; }
 
 #pragma omp declare target
 int doo() { return 0; }
 #pragma omp end declare target
 int car() { return 0; }
 #pragma omp declare target (bar)
 int caz() { return 0; }
 
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[FOO:@.*foo.*]]()
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[BAR:@.*bar.*]]()
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[BAZ:@.*baz.*]]()
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[DOO:@.*doo.*]]()
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[CAR:@.*car.*]]()
-// DEVICE-DAG: define{{ hidden | }}noundef i32 [[CAZ:@.*caz.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[FOO:@.*foo.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[BAR:@.*bar.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[BAZ:@.*baz.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[DOO:@.*doo.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[CAR:@.*car.*]]()
+// DEVICE-DAG: define{{ protected | }}noundef i32 [[CAZ:@.*caz.*]]()
 
 static int c = foo() + bar() + baz();
 #pragma omp declare target (c)
 // HOST-DAG: @[[C_CTOR:__omp_offloading__.+_c_l44_ctor]] = private constant i8 0
 // DEVICE-DAG: define internal void [[C_CTOR:@__omp_offloading__.+_c_l44_ctor]]()
 // DEVICE-DAG: call noundef i32 [[FOO]]()
 // DEVICE-DAG: call noundef i32 [[BAR]]()
 // DEVICE-DAG: call noundef i32 [[BAZ]]()

clang/test/OpenMP/nvptx_target_pure_deleted_codegen.cpp

 // Test target codegen - host bc file has to be created first.
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown -fopenmp-targets=nvptx-nvidia-cuda -emit-llvm-bc %s -o %t-x86-host.bc
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx-unknown-unknown -fopenmp-targets=nvptx-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-x86-host.bc -o - -fno-rtti | FileCheck %s
 // RUN: %clang_cc1 -verify -fopenmp -fexceptions -fcxx-exceptions -x c++ -triple nvptx-unknown-unknown -fopenmp-targets=nvptx-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-x86-host.bc -o - -fno-rtti | FileCheck %s
 // expected-no-diagnostics
 
 #ifndef HEADER
 #define HEADER
 
 // CHECK-NOT: class_type_info
-// CHECK-DAG: @_ZTV7Derived = linkonce_odr hidden unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%class.Derived*)* @_ZN7Derived3fooEv to i8*)] }
-// CHECK-DAG: @_ZTV4Base = linkonce_odr hidden unnamed_addr constant { [3 x i8*] } zeroinitializer
+// CHECK-DAG: @_ZTV7Derived = linkonce_odr protected unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%class.Derived*)* @_ZN7Derived3fooEv to i8*)] }
+// CHECK-DAG: @_ZTV4Base = linkonce_odr protected unnamed_addr constant { [3 x i8*] } zeroinitializer
 // CHECK-NOT: class_type_info
 class Base {
   public:
   virtual void foo() = 0;
 };
 
 class Derived : public Base {
 public:

clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp

struct T1 {
   char c;
   T1() : a(12), f(15) {}
   T1 &operator+(T1 &b) { f += b.a; return *this;}
 };
 
 #pragma omp declare target
 T a = T();
 T f = a;
-// CHECK: define{{ hidden | }}void @{{.+}}foo{{.+}}([[T]]* noundef byval([[T]]) align {{.+}})
+// CHECK: define{{ protected | }}void @{{.+}}foo{{.+}}([[T]]* noundef byval([[T]]) align {{.+}})
 void foo(T a = T()) {
   return;
 }
-// CHECK: define{{ hidden | }}[6 x i64] @{{.+}}bar{{.+}}()
+// CHECK: define{{ protected | }}[6 x i64] @{{.+}}bar{{.+}}()
 T bar() {
 // CHECK:      bitcast [[T]]* %{{.+}} to [6 x i64]*
 // CHECK-NEXT: load [6 x i64], [6 x i64]* %{{.+}},
 // CHECK-NEXT: ret [6 x i64]
   return T();
 }
-// CHECK: define{{ hidden | }}void @{{.+}}baz{{.+}}()
+// CHECK: define{{ protected | }}void @{{.+}}baz{{.+}}()
 void baz() {
 // CHECK:      call [6 x i64] @{{.+}}bar{{.+}}()
 // CHECK-NEXT: bitcast [[T]]* %{{.+}} to [6 x i64]*
 // CHECK-NEXT: store [6 x i64] %{{.+}}, [6 x i64]* %{{.+}},
   T t = bar();
 }
 T1 a1 = T1();
 T1 f1 = a1;
-// CHECK: define{{ hidden | }}void @{{.+}}foo1{{.+}}([[T1]]* noundef byval([[T1]]) align {{.+}})
+// CHECK: define{{ protected | }}void @{{.+}}foo1{{.+}}([[T1]]* noundef byval([[T1]]) align {{.+}})
 void foo1(T1 a = T1()) {
   return;
 }
-// CHECK: define{{ hidden | }}[[T1]] @{{.+}}bar1{{.+}}()
+// CHECK: define{{ protected | }}[[T1]] @{{.+}}bar1{{.+}}()
 T1 bar1() {
 // CHECK:      load [[T1]], [[T1]]*
 // CHECK-NEXT: ret [[T1]]
   return T1();
 }
-// CHECK: define{{ hidden | }}void @{{.+}}baz1{{.+}}()
+// CHECK: define{{ protected | }}void @{{.+}}baz1{{.+}}()
 void baz1() {
 // CHECK: call [[T1]] @{{.+}}bar1{{.+}}()
   T1 t = bar1();
 }
 #pragma omp end declare target
 

clang/test/OpenMP/target_attribute_convergent.cpp

 // 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-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-device -o - | FileCheck %s
 // expected-no-diagnostics
 
 #pragma omp declare target
 
 void foo() {}
 
 #pragma omp end declare target
 
 // CHECK: Function Attrs: {{.*}}convergent{{.*}}
-// CHECK: define hidden void @_Z3foov() [[ATTRIBUTE_NUMBER:#[0-9]+]]
+// CHECK: define protected void @_Z3foov() [[ATTRIBUTE_NUMBER:#[0-9]+]]
 // CHECK: attributes [[ATTRIBUTE_NUMBER]] = { {{.*}}convergent{{.*}} }