FYI, llvm.global_dtor fix is in https://reviews.llvm.org/D66373

In D64943#1633372, @lebedev.ri wrote:

In D64943#1633357, @vzakhari wrote:

In D64943#1633175, @ABataev wrote:

In D64943#1633170, @lebedev.ri wrote:

In D64943#1633164, @ABataev wrote:

In 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
Author: Reid Kleckner <rnk@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.

In D64943#1633175, @ABataev wrote:

In D64943#1633170, @lebedev.ri wrote:

In D64943#1633164, @ABataev wrote:

In 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 changed the tests to use FileCheck variables whenever possible.

In D63846#1574302, @rjmccall wrote:

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

In D63846#1574102, @rjmccall wrote:

Please don't check IR names in test output. That actually includes anonymous names like %2; these should always be tested with FileCheck variables. I suggest using %.* as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems with accidentally matching too much.

ping

Adding more reviewers.

ping

OK, this should work while clang_generate_header() is used for plain files.

In D58537#1476570, @tstellar wrote:

Can you test D61054?

ping

I think I got it, finally, after talking with HJ. As defined in https://github.com/hjl-tools/x86-psABI/blob/hjl/master/dl.tex, the pr_data field consists of 4-byte integers for GNU_PROPERTY_X86_FEATURE_1_AND program property type (pr_type). At the same time, the program property array must be a multiple of 8 bytes on x86-64 (as stated in https://github.com/hjl-tools/linux-abi/blob/hjl/master/object-files.tex), thus, the alignment is computed properly.

Please submit your patch with full context according to LLVM's documentation: https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

In D56108#1341746, @Hahnfeld wrote:

Can you explain the benefit of this change? I would have expected a target like install-omptarget, but applying this patch locally didn't change anything visible.

The incremenetal fix.

ping

The new EOF handling looks much better to me. I added a couple of negative tests for C-style comments and code blocks crossing the file boundary. I also renamed include files from .td to .inc so that they are not considered as real tablegen tests.

In D53840#1282024, @nhaehnle wrote:

I have to say I'm feeling a bit ambivalent about this. I'd say it would be nicer to have a mechanism that integrates with the rest of the TableGen language, but that's admittedly non-trivial. So I guess this approach is okay.

Thank you for the prompt reply, Nikolai! Yes, I decided to keep the preprocessing aside to minimize changes in the actual lexing.

Updated diff with the context.