[PATCH] D108421: Mark openmp internal global dso_local

Tue Aug 24 04:56:16 PDT 2021

ABataev added a comment.

In D108421#2962199 <https://reviews.llvm.org/D108421#2962199>, @lebedev.ri wrote:

> In D108421#2962160 <https://reviews.llvm.org/D108421#2962160>, @ABataev wrote:
>
>> In D108421#2962151 <https://reviews.llvm.org/D108421#2962151>, @lebedev.ri wrote:
>>
>>> In D108421#2961611 <https://reviews.llvm.org/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