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

In D108421#2977107 <https://reviews.llvm.org/D108421#2977107>, @kamleshbhalui 
wrote:

> In D108421#2958848 <https://reviews.llvm.org/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

Reply via email to