[Openmp-commits] [PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Vyacheslav Zakharin via Phabricator via Openmp-commits]

vzakhari added a comment.

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

> In D64943#1633357 <https://reviews.llvm.org/D64943#1633357>, @vzakhari wrote:
>
> > In D64943#1633175 <https://reviews.llvm.org/D64943#1633175>, @ABataev wrote:
> >
> > > In D64943#1633170 <https://reviews.llvm.org/D64943#1633170>, @lebedev.ri wrote:
> > >
> > > > In D64943#1633164 <https://reviews.llvm.org/D64943#1633164>, @ABataev wrote:
> > > >
> > > > > In D64943#1619958 <https://reviews.llvm.org/D64943#1619958>, @sdmitriev wrote:
> > > > >
> > > > > > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ on Linux and ‘atexit’ on Windows) whereas @llvm.global_ctors/dtors variables offer similar and platform neutral functionality (http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable). Why do you think that ‘atexit’ is a better choice?
> > > > >
> > > > >
> > > > > Because it does not work on Windows, better to have portable solution, if possible.
> > > >
> > > >
> > > > Is there a bug # ?
> > >
> > >
> > > @vzakhari?
> >
> >
> > I do not have bug #, but the issue was introduced with the following commit:
> >  commit f803b23879d9e1d9415ec1875713534dcc203df5 <https://reviews.llvm.org/rGf803b23879d9e1d9415ec1875713534dcc203df5>
> >  Author: Reid Kleckner <rnk at google.com>
> >  Date:   Fri Sep 7 23:07:55 2018 +0000
> >
> >   [COFF] Implement llvm.global_ctors priorities for MSVC COFF targets
> >   
> >   Summary:
> >   MSVC and LLD sort sections ASCII-betically, so we need to use section
> >   names that sort between .CRT$XCA (the start) and .CRT$XCU (the default
> >   priority).
> >   
> >   In the general case, use .CRT$XCT12345 as the section name, and let the
> >   linker sort the zero-padded digits.
> >   
> >   Users with low priorities typically want to initialize as early as
> >   possible, so use .CRT$XCA00199 for prioties less than 200. This number
> >   is arbitrary.
> >   
> >   Implements PR38552.
> >   
> >   Reviewers: majnemer, mstorsjo
> >   
> >   Subscribers: hiraditya, llvm-commits
> >   
> >   Differential Revision: https://reviews.llvm.org/D51820
> >   
> >   llvm-svn: 341727
> >   
> >   
> >
> > The destructors are still in .CRT$XT for default priority (65535) now, but for non-default priority they will go into .CRT$XC.  I will upload a fixing patch with a LIT test shortly.
> >
> > This clang-offload-wrapper commit will work properly, if we use default priority for the destructors.
>
>
> 'IMHO' if there is a problem with lowering of LLVM IR constructs for some
>  particular targets, that problem must be resolved instead of adding workarounds.


I completely agree with you!  I am testing the patch for destructors.


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

https://reviews.llvm.org/D64943