Mark openmp internal global dso_local

[kamleshbhalui]

Summary

Starting from clang-12 openmp started generating internal global variable with got relocation even when static relocation enabled.
In clang-11 shouldAssumeDSOLocal was assuming it dso_local based on static relocation model.
Since shouldAssumeDSOLocal is cleaned up now for respecting dso_local generated from frontend, marking openmp internal globals as dso_local.

[kamleshbhalui]

clang formatted

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2186–2187

I would not do it here since the linkage is changing in some places. Better to make this kind of change explicitly where required + need to add tests.

[kamleshbhalui]

updated test and make changes local to auto generated global vars for lock.

[kamleshbhalui]

assume dso local if relocation model static

[lebedev.ri]

In D108421#2958742, @kamleshbhalui wrote:

assume dso local if relocation model static

I think these are two separate fixes. TargetMachine::shouldAssumeDSOLocal() change might make sense,
but as the comment there notes, this really should be fixed in in CGOpenMPRuntime::getOrCreateInternalVariable() itself (is that not the right default?),
and in it's every user that incorrectly unmarks said global as being `dso_local.

[kamleshbhalui]

In D108421#2958745, @lebedev.ri wrote:

In D108421#2958742, @kamleshbhalui wrote:

assume dso local if relocation model static

I think these are two separate fixes. TargetMachine::shouldAssumeDSOLocal() change might make sense,
but as the comment there notes, this really should be fixed in in CGOpenMPRuntime::getOrCreateInternalVariable() itself (is that not the right default?),
and in it's every user that incorrectly unmarks said global as being `dso_local.

Please see this revision https://reviews.llvm.org/differential/diff/367953/
Should I get back to this?

[MaskRay]

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the wrong direction. dso_local is assumed to be marked by the frontend.

Direct accesses and GOT accesses are trade-offs. Direct accesses are not always preferred. The MachO special case is an unfortunate case for their xnu kernel, not a good example here.

[kamleshbhalui]

updated test and make changes local to auto generated global vars for lock.

[MaskRay]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2194

Can be variable be preemptible on ELF? (i.e. default visibility non-local linkage) If yes, it cannot be marked dso_local in that case.

2194

in that case

> When -fpic is used.

-fpic -shared may give a linker error.

[lebedev.ri]

In D108421#2961611, @kamleshbhalui wrote:

updated test and make changes local to auto generated global vars for lock.

I do not understand why the most original diff here was wrong.
Is is the wrong default for getOrCreateInternalVariable() to default to dso_local?

[ABataev]

In D108421#2962151, @lebedev.ri wrote:

In D108421#2961611, @kamleshbhalui wrote:

updated test and make changes local to auto generated global vars for lock.

I do not understand why the most original diff here was wrong.
Is is the wrong default for getOrCreateInternalVariable() to default to dso_local?

Need to check that the linkage/visibility of the created vars is not changing in place. If it is not changing, then it is good to go. Otherwise, need to add some another function/extra parameter with linkage/visibility settings.

[lebedev.ri]

In D108421#2962160, @ABataev wrote:

In D108421#2962151, @lebedev.ri wrote:

In D108421#2961611, @kamleshbhalui wrote:

updated test and make changes local to auto generated global vars for lock.

I do not understand why the most original diff here was wrong.
Is is the wrong default for getOrCreateInternalVariable() to default to dso_local?

Need to check that the linkage/visibility of the created vars is not changing in place. If it is not changing, then it is good to go. Otherwise, need to add some another function/extra parameter with linkage/visibility settings.

I'm having trouble parsing the comment, but i don't really see why the fact that some users may be changing linkage
should deter us from having the right default. If some users change linkage, then regardless of where this is dealt with,
the users will have to be adjusted, just a different set of them. It seems to me that fixing the ones that already mess with linkage is the right choice,
not fixing the ones that *don't* mess with linkage.

[ABataev]

In D108421#2962199, @lebedev.ri wrote:

In D108421#2962160, @ABataev wrote:

In D108421#2962151, @lebedev.ri wrote:

In D108421#2961611, @kamleshbhalui wrote:

updated test and make changes local to auto generated global vars for lock.

I do not understand why the most original diff here was wrong.
Is is the wrong default for getOrCreateInternalVariable() to default to dso_local?

Need to check that the linkage/visibility of the created vars is not changing in place. If it is not changing, then it is good to go. Otherwise, need to add some another function/extra parameter with linkage/visibility settings.

I'm having trouble parsing the comment, but i don't really see why the fact that some users may be changing linkage
should deter us from having the right default.

Yes, but the fix should not introduce bugs, if possible. Need to check the users and make them work correctly too, that's why need to check the users and if some of them change the linkage/visibility, then need to add a new function or add an extra parameter to track whether the need to add dso_local attribute.

If some users change linkage, then regardless of where this is dealt with, the users will have to be adjusted, just a different set of them. It seems to me that fixing the ones that already mess with linkage is the right choice, not fixing the ones that *don't* mess with linkage.

That's all I'm asking, actually. Or just keep the original code (if no time to go through the users) and fix dso_local only where required.

[kamleshbhalui]

In D108421#2958848, @MaskRay wrote:

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the wrong direction. dso_local is assumed to be marked by the frontend.

Direct accesses and GOT accesses are trade-offs. Direct accesses are not always preferred. The MachO special case is an unfortunate case for their xnu kernel, not a good example here.

@MaskRay I would like to know more about these trade-offs details, any supporting documents will do.
Considering below testcase, can you shed some light how code generated by llc-12 is better than llc-11 for given testcase?
https://godbolt.org/z/x9xojWb58
And FYI this testcase does not work when build as Linux Kernel Module. LKM loader throw error("Unknown rela relocation: 42")?

[MaskRay]

In D108421#2977107, @kamleshbhalui wrote:

In D108421#2958848, @MaskRay wrote:

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the wrong direction. dso_local is assumed to be marked by the frontend.

Direct accesses and GOT accesses are trade-offs. Direct accesses are not always preferred. The MachO special case is an unfortunate case for their xnu kernel, not a good example here.

@MaskRay I would like to know more about these trade-offs details, any supporting documents will do.
Considering below testcase, can you shed some light how code generated by llc-12 is better than llc-11 for given testcase?
https://godbolt.org/z/x9xojWb58

This is a very minor issue. First, global variable access is rarely a performance bottleneck.
Second, if the symbol turns out to be non-preemptible (which implies that it is defined in the component), the R_X86_64_REX_GOTPCRELX GOT indirection can be optimized out.
The only minor issue is slightly longer code sequence.

And FYI this testcase does not work when build as Linux Kernel Module. LKM loader throw error("Unknown rela relocation: 42")?

This is a kernel issue. Please mention the justification (is it related to OpenMP?) in the first place.
The kernel can be compiled in -fpie mode. In this mode, taking the address of a default-visibility undefined symbol uses R_X86_64_REX_GOTPCRELX.
So the kernel should support R_X86_64_REX_GOTPCRELX anyway.

---

If we think the optimization is meaningful:

Depending on the property of .gomp_critical_user_.atomic_reduction.var different DSOLocal strategies should be used.
If it is TU-local, make it local linkage. If it is linked image local, make it hidden visibility.
If it may be defined in a shared object and shared with other shared objects or the main executable, (not so sure because such symbol interposition does not work in other binary formats), use dso_preemptable as is.

I believe the current forced dso_local is definitely incorrect because it may break -fpic -shared links.

[umesh.kalappa0]

In D108421#2977216, @MaskRay wrote:

In D108421#2977107, @kamleshbhalui wrote:

In D108421#2958848, @MaskRay wrote:

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the wrong direction. dso_local is assumed to be marked by the frontend.

Direct accesses and GOT accesses are trade-offs. Direct accesses are not always preferred. The MachO special case is an unfortunate case for their xnu kernel, not a good example here.

@MaskRay I would like to know more about these trade-offs details, any supporting documents will do.
Considering below testcase, can you shed some light how code generated by llc-12 is better than llc-11 for given testcase?
https://godbolt.org/z/x9xojWb58

This is a very minor issue. First, global variable access is rarely a performance bottleneck.
Second, if the symbol turns out to be non-preemptible (which implies that it is defined in the component), the R_X86_64_REX_GOTPCRELX GOT indirection can be optimized out.
The only minor issue is slightly longer code sequence.

And FYI this testcase does not work when build as Linux Kernel Module. LKM loader throw error("Unknown rela relocation: 42")?

This is a kernel issue. Please mention the justification (is it related to OpenMP?) in the first place.
The kernel can be compiled in -fpie mode. In this mode, taking the address of a default-visibility undefined symbol uses R_X86_64_REX_GOTPCRELX.
So the kernel should support R_X86_64_REX_GOTPCRELX anyway.

---

If we think the optimization is meaningful:

Depending on the property of .gomp_critical_user_.atomic_reduction.var different DSOLocal strategies should be used.
If it is TU-local, make it local linkage. If it is linked image local, make it hidden visibility.
If it may be defined in a shared object and shared with other shared objects or the main executable, (not so sure because such symbol interposition does not work in other binary formats), use dso_preemptable as is.

I believe the current forced dso_local is definitely incorrect because it may break -fpic -shared links.

@kamleshbhalui ,make the changes accordingly that honour -fpic and don't mark dsolocal in this case.

[kamleshbhalui]

mark dso_local only when non-pic

[kamleshbhalui]

ping?

[kamleshbhalui]

ping?

clang/lib/CodeGen/CGOpenMPRuntime.cpp

assert(Elem.second->getType()->getPointerElementType() == Ty &&
            "OMP internal variable has different type than requested");
     return &*Elem.second;
   }
 
   return Elem.second = new llvm::GlobalVariable(
              CGM.getModule(), Ty, /*IsConstant*/ false,
              llvm::GlobalValue::CommonLinkage, llvm::Constant::getNullValue(Ty),
              Elem.first(), /*InsertBefore=*/nullptr,
              llvm::GlobalValue::NotThreadLocal, AddressSpace);
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {
   std::string Prefix = Twine("gomp_critical_user_", CriticalName).str();
   std::string Name = getName({Prefix, "var"});
-  return getOrCreateInternalVariable(KmpCriticalNameTy, Name);
+  llvm::GlobalVariable *GV = cast<llvm::GlobalVariable>(
+      getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+    GV->setDSOLocal(true);
+
+  return cast<llvm::Constant>(GV);
 }
 
 namespace {
 /// Common pre(post)-action for different OpenMP constructs.
 class CommonActionTy final : public PrePostActionTy {
   llvm::FunctionCallee EnterCallee;
   ArrayRef<llvm::Value *> EnterArgs;
   llvm::FunctionCallee ExitCallee;

clang/test/OpenMP/critical_codegen.cpp

 // RUN: %clang_cc1 -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -fopenmp-simd -fexceptions -fcxx-exceptions -debug-info-kind=line-tables-only -x c++ -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // SIMD-ONLY0-NOT: {{__kmpc|__tgt}}
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
 // ALL:       [[IDENT_T_TY:%.+]] = type { i32, i32, i32, i32, i8* }
-// ALL:       [[UNNAMED_LOCK:@.+]] = common global [8 x i32] zeroinitializer
-// ALL:       [[THE_NAME_LOCK:@.+]] = common global [8 x i32] zeroinitializer
-// ALL:       [[THE_NAME_LOCK1:@.+]] = common global [8 x i32] zeroinitializer
+// ALL:       [[UNNAMED_LOCK:@.+]] = common dso_local global [8 x i32] zeroinitializer
+// ALL:       [[THE_NAME_LOCK:@.+]] = common dso_local global [8 x i32] zeroinitializer
+// ALL:       [[THE_NAME_LOCK1:@.+]] = common dso_local global [8 x i32] zeroinitializer
 
 // ALL:       define {{.*}}void [[FOO:@.+]]()
 
 void foo() { extern void mayThrow(); mayThrow(); }
 
 // ALL-LABEL: @main
 // TERM_DEBUG-LABEL: @main
 int main() {

clang/test/OpenMP/critical_codegen_attr.cpp

 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=51 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -fopenmp-simd -fopenmp-version=51 -fexceptions -fcxx-exceptions -debug-info-kind=line-tables-only -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // SIMD-ONLY0-NOT: {{__kmpc|__tgt}}
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
 // ALL:       [[IDENT_T_TY:%.+]] = type { i32, i32, i32, i32, i8* }
-// ALL:       [[UNNAMED_LOCK:@.+]] = common global [8 x i32] zeroinitializer
-// ALL:       [[THE_NAME_LOCK:@.+]] = common global [8 x i32] zeroinitializer
-// ALL:       [[THE_NAME_LOCK1:@.+]] = common global [8 x i32] zeroinitializer
+// ALL:       [[UNNAMED_LOCK:@.+]] = common dso_local global [8 x i32] zeroinitializer
+// ALL:       [[THE_NAME_LOCK:@.+]] = common dso_local global [8 x i32] zeroinitializer
+// ALL:       [[THE_NAME_LOCK1:@.+]] = common dso_local global [8 x i32] zeroinitializer
 
 // ALL:       define {{.*}}void [[FOO:@.+]]()
 
 void foo() { extern void mayThrow(); mayThrow(); }
 
 // ALL-LABEL: @main
 // TERM_DEBUG-LABEL: @main
 int main() {

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Constant *OpenMPIRBuilder::getOrCreateOMPInternalVariable(
   }
 
   return Elem.second;
 }
 
 Value *OpenMPIRBuilder::getOMPCriticalRegionLock(StringRef CriticalName) {
   std::string Prefix = Twine("gomp_critical_user_", CriticalName).str();
   std::string Name = getNameWithSeparators({Prefix, "var"}, ".", ".");
-  return getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name);
+  llvm::GlobalVariable *GV = cast<llvm::GlobalVariable>(
+      getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+    GV->setDSOLocal(true);
+
+  return cast<llvm::Constant>(GV);
 }
 
 GlobalVariable *
 OpenMPIRBuilder::createOffloadMaptypes(SmallVectorImpl<uint64_t> &Mappings,
                                        std::string VarName) {
   llvm::Constant *MaptypesArrayInit =
       llvm::ConstantDataArray::get(M.getContext(), Mappings);
   auto *MaptypesArrayGlobal = new llvm::GlobalVariable(