[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

ping?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

ping?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui updated this revision to Diff 370837.
kamleshbhalui added a comment.

mark dso_local only when non-pic


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/OpenMP/critical_codegen.cpp
  clang/test/OpenMP/critical_codegen_attr.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2487,7 +2487,13 @@
 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(
+  getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  bool IsPIE = GV->getParent()->getPIELevel() != PIELevel::Default;
+  bool IsPIC = GV->getParent()->getPICLevel() != PICLevel::NotPIC;
+  GV->setDSOLocal(!IsPIC || IsPIE);
+
+  return cast(GV);
 }
 
 GlobalVariable *
Index: clang/test/OpenMP/critical_codegen_attr.cpp
===
--- clang/test/OpenMP/critical_codegen_attr.cpp
+++ clang/test/OpenMP/critical_codegen_attr.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/test/OpenMP/critical_codegen.cpp
===
--- clang/test/OpenMP/critical_codegen.cpp
+++ clang/test/OpenMP/critical_codegen.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -208,6 +208,13 @@
 ModuleNameHash = (Twine(".__uniq.") +
 Twine(toString(IntHash, /* Radix = */ 10, /* Signed = */false))).str();
   }
+
+  if (uint32_t PLevel = Context.getLangOpts().PICLevel) {
+assert(PLevel < 3 && "Invalid PIC Level");
+getModule().setPICLevel(static_cast(PLevel));
+if (Context.getLangOpts().PIE)
+  getModule().setPIELevel(static_cast(PLevel));
+  }
 }
 
 CodeGenModule::~CodeGenModule() {}
@@ -745,13 +752,6 @@
 }
   }
 
-  if (uint32_t PLevel = Context.getLangOpts().PICLevel) {
-assert(PLevel < 3 && "Invalid PIC Level");
-getModule().setPICLevel(static_cast(PLevel));
-if (Context.getLangOpts().PIE)
-  getModule().setPIELevel(static_cast(PLevel));
-  }
-
   if (getCodeGenOpts().CodeModel.size() > 0) {
 unsigned CM = llvm::StringSwitch(getCodeGenOpts().CodeModel)
   .Case("tiny", llvm::CodeModel::Tiny)
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2189,7 +2189,12 @@
 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(
+  getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  bool IsPIE = GV->getParent()->getPIELevel() != llvm::PIELevel::Default;
+  bool IsPIC = GV->getParent()->getPICLevel() != llvm::PICLevel::NotPIC;
+  GV->setDSOLocal(!IsPIC || IsPIE);
+  return cast(GV);
 }
 
 namespace {

[PATCH] D108421: Mark openmp internal global dso_local

[Umesh Kalappa via Phabricator via cfe-commits]

umesh.kalappa0 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Fangrui Song via Phabricator via cfe-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

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")?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2194
+  getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);

MaskRay wrote:
> 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.
> in that case

=> When -fpic is used.

`-fpic -shared` may give a linker error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2194
+  getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui updated this revision to Diff 368261.
kamleshbhalui added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/critical_codegen.cpp
  clang/test/OpenMP/critical_codegen_attr.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp


Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2487,7 +2487,12 @@
 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(
+  getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 GlobalVariable *
Index: clang/test/OpenMP/critical_codegen_attr.cpp
===
--- clang/test/OpenMP/critical_codegen_attr.cpp
+++ clang/test/OpenMP/critical_codegen_attr.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/test/OpenMP/critical_codegen.cpp
===
--- clang/test/OpenMP/critical_codegen.cpp
+++ clang/test/OpenMP/critical_codegen.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2189,7 +2189,12 @@
 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(
+  getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 namespace {


Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2487,7 +2487,12 @@
 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(
+  getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 GlobalVariable *
Index: clang/test/OpenMP/critical_codegen_attr.cpp
===
--- clang/test/OpenMP/critical_codegen_attr.cpp
+++ clang/test/OpenMP/critical_codegen_attr.cpp
@@ -16,9 +16,9 @@
 #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:  

[PATCH] D108421: Mark openmp internal global dso_local

[Fangrui Song via Phabricator via cfe-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the 
wrong direction. dso_local is assumed to 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui updated this revision to Diff 367954.
kamleshbhalui added a comment.

assume dso local if relocation model static


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

Files:
  llvm/lib/Target/TargetMachine.cpp


Index: llvm/lib/Target/TargetMachine.cpp
===
--- llvm/lib/Target/TargetMachine.cpp
+++ llvm/lib/Target/TargetMachine.cpp
@@ -149,6 +149,11 @@
 return GV->isStrongDefinitionForLinker();
   }
 
+  if (TT.isOSBinFormatELF()) {
+if (RM == Reloc::Static)
+  return true;
+  }
+
   // Due to the AIX linkage model, any global with default visibility is
   // considered non-local.
   if (TT.isOSBinFormatXCOFF())


Index: llvm/lib/Target/TargetMachine.cpp
===
--- llvm/lib/Target/TargetMachine.cpp
+++ llvm/lib/Target/TargetMachine.cpp
@@ -149,6 +149,11 @@
 return GV->isStrongDefinitionForLinker();
   }
 
+  if (TT.isOSBinFormatELF()) {
+if (RM == Reloc::Static)
+  return true;
+  }
+
   // Due to the AIX linkage model, any global with default visibility is
   // considered non-local.
   if (TT.isOSBinFormatXCOFF())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui updated this revision to Diff 367953.
kamleshbhalui added a comment.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/critical_codegen.cpp
  clang/test/OpenMP/critical_codegen_attr.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp


Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2487,7 +2487,12 @@
 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(
+  getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 GlobalVariable *
Index: clang/test/OpenMP/critical_codegen_attr.cpp
===
--- clang/test/OpenMP/critical_codegen_attr.cpp
+++ clang/test/OpenMP/critical_codegen_attr.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/test/OpenMP/critical_codegen.cpp
===
--- clang/test/OpenMP/critical_codegen.cpp
+++ clang/test/OpenMP/critical_codegen.cpp
@@ -16,9 +16,9 @@
 #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:@.+]]()
 
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2189,7 +2189,12 @@
 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(
+  getOrCreateInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 namespace {


Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2487,7 +2487,12 @@
 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(
+  getOrCreateOMPInternalVariable(KmpCriticalNameTy, Name));
+  if (!GV->isDSOLocal())
+GV->setDSOLocal(true);
+
+  return cast(GV);
 }
 
 GlobalVariable *
Index: clang/test/OpenMP/critical_codegen_attr.cpp
===
--- clang/test/OpenMP/critical_codegen_attr.cpp
+++ clang/test/OpenMP/critical_codegen_attr.cpp
@@ -16,9 +16,9 @@
 #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:   

[PATCH] D108421: Mark openmp internal global dso_local

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2182-2189
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
+  CGM.getModule(), Ty, /*IsConstant*/ false,
+  llvm::GlobalValue::CommonLinkage, llvm::Constant::getNullValue(Ty),
+  Elem.first(), /*InsertBefore=*/nullptr, 
llvm::GlobalValue::NotThreadLocal,
+  AddressSpace);
+  GV->setDSOLocal(true);
+  Elem.second = GV;

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui updated this revision to Diff 367700.
kamleshbhalui added a comment.

clang formatted


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108421/new/

https://reviews.llvm.org/D108421

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2179,11 +2179,14 @@
 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::GlobalVariable *GV = new llvm::GlobalVariable(
+  CGM.getModule(), Ty, /*IsConstant*/ false,
+  llvm::GlobalValue::CommonLinkage, llvm::Constant::getNullValue(Ty),
+  Elem.first(), /*InsertBefore=*/nullptr, 
llvm::GlobalValue::NotThreadLocal,
+  AddressSpace);
+  GV->setDSOLocal(true);
+  Elem.second = GV;
+  return Elem.second;
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2179,11 +2179,14 @@
 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::GlobalVariable *GV = new llvm::GlobalVariable(
+  CGM.getModule(), Ty, /*IsConstant*/ false,
+  llvm::GlobalValue::CommonLinkage, llvm::Constant::getNullValue(Ty),
+  Elem.first(), /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+  AddressSpace);
+  GV->setDSOLocal(true);
+  Elem.second = GV;
+  return Elem.second;
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108421: Mark openmp internal global dso_local

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui created this revision.
kamleshbhalui added reviewers: MaskRay, pengfei.
kamleshbhalui added a project: OpenMP.
Herald added subscribers: guansong, yaxunl.
kamleshbhalui requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108421

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2179,11 +2179,14 @@
 return &*Elem.second;
   }
 
-  return Elem.second = new llvm::GlobalVariable(
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
  CGM.getModule(), Ty, /*IsConstant*/ false,
  llvm::GlobalValue::CommonLinkage, 
llvm::Constant::getNullValue(Ty),
  Elem.first(), /*InsertBefore=*/nullptr,
  llvm::GlobalValue::NotThreadLocal, AddressSpace);
+  GV->setDSOLocal(true);
+  Elem.second = GV;
+  return Elem.second;
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2179,11 +2179,14 @@
 return &*Elem.second;
   }
 
-  return Elem.second = new llvm::GlobalVariable(
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
  CGM.getModule(), Ty, /*IsConstant*/ false,
  llvm::GlobalValue::CommonLinkage, llvm::Constant::getNullValue(Ty),
  Elem.first(), /*InsertBefore=*/nullptr,
  llvm::GlobalValue::NotThreadLocal, AddressSpace);
+  GV->setDSOLocal(true);
+  Elem.second = GV;
+  return Elem.second;
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits