Fixed clang-format remarks.

In D95572#2527083, @jdoerfert wrote:

In D95572#2527082, @vzakhari wrote:

In D95572#2527005, @grokos wrote:

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

Do you mean I can ignore the pre-merge checks fail? I do not see how my changes could have affected libomp build.

looks to me like that is the C++ dependence problem we discussed this morning. I don't think this patch caused it so I would go ahead.

In D95572#2527005, @grokos wrote:

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

In D95483#2525899, @RaviNarayanaswamy wrote:

In D95483#2525182, @JonChesterfield wrote:

There are a bunch of side channels for communicating between the compiler and the runtime already. Compiler emitted magic symbols, a msgpack blob for amdgpu, writing extra stuff into the elf in general etc.

What information do you intend to pass along through this new channel, why yaml, and why for all plugins?

Yes there may be many ways to communicate information from compiler to runtime. The proposal is one way and we others wanted to communicate information to runtime and come up with a standard scheme.
We added this to other plugins to show how it can be done. Plugins can opt in,, just dont add the unique signature and the header.

offloading/dynamic_module_load.c fails due to this commit in my environment with x86_64 offload. It fails because std::call_once cannot be used without libpthread, and libomptarget.rtl.x86_64.so is not linked with libpthread. Basically, gdb shows that include/c++/7.3.0/x86_64-linux-gnu/bits/gthr-default.h::__gthread_active_p() returns false, when std::call_once is called from libomptarget.rtl.x86_64.so. It does return true, though, when std::call_once is called from libomptarget.so, because libomptarget.so is linked with libpthread.

In D93727#2523723, @jhuber6 wrote:

In D93727#2523719, @JonChesterfield wrote:

Can mark the function itself as ((used)), for slightly less typing. Seems a reasonable workaround to me, with a comment like 'workaround bug in gcc 5.ish'

Yeah, that's an option. Adding a dummy variable would defeat the original purpose of initializing the variables this way, rather than having each plugin initialize it individually and globally. This problem only seems to occur on GCC x86 from the compilers I've checked. The reason It wasn't failing on my machine was because it was GCC 6.3 on PPC. I'm assuming any other compilers will just ignore the attribute if they don't understand it. This should be sufficient.

static inline uint32_t __attribute__((used)) getInfoLevel  () { ... }
static inline uint32_t __attribute__((used)) getDebugLevel  () { ... }

LGTM

Thank you, @ggeorgakoudis !

Hi @ggeorgakoudis, please try something along these lines:

In D93055#2516898, @ggeorgakoudis wrote:

In D93055#2516351, @vzakhari wrote:

FYI, libomptarget LIT tests fail with profiling enabled in shared-libs builds, because libLLVMSupport.so.12git dependency cannot be resolved. Apparently, LD_LIBRARY_PATH is only set to <build>/llvm/projects/openmp/libomptarget, which obviously does not contain the support library.

Thanks for reporting that @vzakhari. Will fix soon.

In D93055#2516823, @kkwli0 wrote:

In D93055#2516351, @vzakhari wrote:

FYI, libomptarget LIT tests fail with profiling enabled in shared-libs builds, because libLLVMSupport.so.12git dependency cannot be resolved. Apparently, LD_LIBRARY_PATH is only set to <build>/llvm/projects/openmp/libomptarget, which obviously does not contain the support library.

@vzakhari Is it an out-of-tree build? If it is, you need to set LIBOMPTARGET_OPENMP_HEADER_FOLDER and DLIBOMPTARGET_OPENMP_HOST_RTL_FOLDER (https://github.com/llvm/llvm-project/tree/main/openmp#id11).

FYI, libomptarget LIT tests fail with profiling enabled in shared-libs builds, because libLLVMSupport.so.12git dependency cannot be resolved. Apparently, LD_LIBRARY_PATH is only set to <build>/llvm/projects/openmp/libomptarget, which obviously does not contain the support library.

Should be working now. Thanks!

FYI, the tests pass with additional changes in lld/MachO/SyntheticSections.cpp.

Thank you! The patch does not build, since some CPP definitions are still using size_t.

In D80177#2097037, @int3 wrote:

rGd767de44bf9527cb5058f5fe16aac2f23c21977c was the referenced fix. Good point about size_t, I'll change that

In D80177#2069811, @int3 wrote:

Thanks for the heads up, just pushed a likely fix and will monitor http://lab.llvm.org:8011/builders/lld-x86_64-win. I'll set up a local Windows VM to catch future such issues earlier...

In D77184#2056306, @vzakhari wrote:

It does not work on Windows (msbuild) for me, because ${pathlist_escaped} contains paths like %(build_mode)s/bin (caused by set_llvm_build_mode). This seems to break something so that python process does not produce any output at all: I checked both OUTPUT_VARIABLE and ERROR_VARIABLE and they were empty on any invocation of execute_process. @thakis, do you have any ideas what might be wrong?

FWIW, the code does not seem to work as expected for multi-configuration environments, since the %(build_mode) substitution is not done yet and the paths are not real filesystem paths.

It does not work on Windows (msbuild) for me, because ${pathlist_escaped} contains paths like %(build_mode)s/bin (caused by set_llvm_build_mode). This seems to break something so that python process does not produce any output at all: I checked both OUTPUT_VARIABLE and ERROR_VARIABLE and they were empty on any invocation of execute_process. @thakis, do you have any ideas what might be wrong?

LGTM

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.