LG

LG

LGTM, thanks

I think at least this patch needs a clang-format, maybe also applies to the other recent patches.

In D96033#2767884, @teemperor wrote:

In D96033#2766502, @phosek wrote:

In D96033#2766372, @v.g.vassilev wrote:

In D96033#2766332, @phosek wrote:

We've started seeing LLVM ERROR: out of memory on our 2-stage LTO Linux builders after this change landed. It looks like linking clang-repl always fails on our bot, but I've also seen OOM when linking ClangCodeGenTests and FrontendTests. Do you have any idea why this could be happening? We'd appreciate any help since our bots have been broken for several days now.

Ouch. Are the bot logs public? If not maybe a stacktrace could be useful. clang-repl combines a lot of libraries across llvm and clang that usually are compiled separately. For instance we put in memory most of the clang frontend, the backend and the JIT. Could it be we are hitting some real limit?

Yes, they are, see https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64, but there isn't much information in there unfortunately. It's possible that we're hitting some limit, but these bots use 32-core instances with 128GB RAM which I'd hope is enough even for the LTO build.

I think the specs are fine for just building with LTO, but I am not sure if that's enough to for the worst case when running ninja -j320 with an LTO build (which is what your job is doing). Can you try limiting your link jobs to something like 16 or 32 (e.g., -DLLVM_PARALLEL_LINK_JOBS=32)

Can we get this fixed somehow? It's annoying that there is a test failure in Clang without building the OpenMP runtime, just because I have CUDA installed on my machine...

friendly ping :)

Superseded by D87413, I think.

Long addressed by D72779, but I can't seem to be able to close this (?)

In D95915#2541132, @tbaeder wrote:

That's what I was looking at right now as well, since using std::call_once() already means the methods can't be const anymore anyway. Might as well just cache the value.

My proposal would be to cache the return value of the three routines in ToolChain. This has the advantage that the values get parsed only once and there is at most one warning. I don't know how this plays with parallelization efforts, but I don't think we should worry about this right now, given the current code.

LGTM, thanks for the changes!

In D93169#2485228, @tianshilei1992 wrote:

In D93169#2484725, @Hahnfeld wrote:

Why is it necessary to write and compile a C program just to parse /proc/cpuinfo? Can this be done directly from CMake?

Unfortunately we can't. CMAKE_SYSTEM_PROCESSOR just reports aarch64.

Why is it necessary to write and compile a C program just to parse /proc/cpuinfo? Can this be done directly from CMake?

Add comment for find_first_existing_vc_file, thanks @scott.linder.

Ping

In D89974#2348176, @tra wrote:

CUDA path is sort of a global configuration parameter for all CUDA compilations. Perhaps we should consider allowing the user to specify a CUDA search path candidate via environment variable. This should allow transparently overriding preferred CUDA path without having to adjust all builds. I can't say I like it, but it seems to be the least bad way (that I can think of ATM) to address the dependency on something that only the end user would know for sure.

In D89974#2348124, @jdoerfert wrote:

I thought, right now we would configure clang with a cuda path XYZ

In D89974#2347979, @Hahnfeld wrote:

In D89974#2347938, @tra wrote:

I think the default should still let clang search for CUDA or require the user to provide correct CUDA path. "Use CUDA path discovered by CMake at build time" should be a non-default configuration option if/when it's needed and appropriate.

I agree here. It's definitely surprising to make it the *first* path because module loading another CUDA version and putting it into PATH is not recognized anymore.

In D89974#2347938, @tra wrote:

One concern I have is that the path we configure during clang's build is not necessarily the right choice for the user of clang we build. It's likely that the clang in the end will be used on a completely different machine.
E.g. official clang builds can not ever provide the same CUDA path for *all* users who end up using them. Requiring the rest to use a special option to make clang work again looks like an overall usability regression to me.

By the way, there's no commit list subscribed which sounds bad. Adding llvm-commits to give people a chance to be aware of this.

In D89915#2346071, @ldionne wrote:

@Hahnfeld Can you advise on the proper way to fix this? From git blame, it seems like you might know.

In D87084#2254700, @jdoerfert wrote:

Other places need to be updated too: data_sharing_init_stack_common for one.

Do we actually use this array? What happens if you make it a single element and update the function above?

In D83268#2135930, @JonChesterfield wrote:

Aside from the API stability concern this looks uncontentious. Removes dead arguments, generally makes things simpler. Thus LGTM.

@Hahnfeld @ABataev - are you sufficiently persuaded that preserving the current interface is not worth the development cost?

This is definitely not NFC and breaks API compatibility (but apparently nobody cares anymore?).

LGTM

The summary also talks about tools/multiplex/tests/*, is that information obsolete?

LGTM

LGTM

In D82963#2128507, @saiislam wrote:

Thanks @Hahnfeld. I realized that LLVM_MAJOR_VERSION was neither getting set in OpenMPTesting.cmake nor was it
inheriting it from anywhere else. So, OPENMP_TEST_COMPILER_VERSION_MAJOR was also getting set as empty, which
was getting propagated to lit by config.test_compiler_features. That is why "clang-11" was not getting recognized
as a valid target by lit-unsupported (though clang-11.0.0 would have worked). This change should fix this issue.

In D82718#2120414, @ye-luo wrote:

In D82718#2120235, @Hahnfeld wrote:

(In general, all patches must be sent to the respective -commits list. This also makes feedback more likely.)

Sorry, I was not aware of -commits list. What is it for exclusively? If there is policy or instruction , please point me.
I added OpenMP project tag. I you watch OpenMP project on differential. I expect you get notifications. Is it not the case?

(In general, all patches must be sent to the respective -commits list. This also makes feedback more likely.)

In D76012#2100421, @sylvestre.ledru wrote:

@protze.joachim @jdoerfert ompt-multiplex.h is installed by default in /usr/include/
others opemp headers are installed in /usr/include/openmp/

LIBOMP_HEADERS_INSTALL_PATH is the internal compiler where for example omp.h. I think the header tool is of relevance outside of Clang, so IMO it should go the general include/ directory.

I don't feel comfortable reviewing this change, maybe @phosek can take a look.

Does -fopenmp-version work with the Intel Compiler? How will this work if the GCC eventually gets support for OpenMP 5.0?

LGTM as well

In D78566#1995119, @jdenny wrote:

I just noticed another issue: ompt tests are skipped if FileCheck is not available. This is configured in openmp/runtime/test/lit.cfg. However, other openmp test suites that use FileCheck don't bother. Why is ompt different?

This patch makes not visible in all of the openmp project's test
suites. In all but libomptarget/test, it should be possible for a
test author to insert not before a use of the lit substitution for
running a test program.

@thakis thanks for taking care so quickly!

In D76780#1942190, @bryanpkc wrote:

In D76780#1942144, @bryanpkc wrote:

Why doesn't the KMP_MB after the store to th_next_waiting guarantee that the unblocked thread sees that store?

Answering my own question, the ARM architecture reference manual states that the DMB instruction ensures that all affected memory accesses by the PE executing the DMB that appear in program order before the DMB and those which originate from a different PE, ...which have been Observed-by the PE before the DMB is executed, are Observed-by each PE, ...before any affected memory accesses that appear in program order after the DMB are Observed-by that PE.

I think this means, in this case, that the store of FALSE to th_spin_here is observed in the correct order only by the releasing thread that issued the DMB. Other threads (e.g. the spinning thread) could still see the updates to th_spin_here and th_next_waiting out of order, unless they also issue DMB, which is the fix in this patch.

Yes, this looks related to memory consistency. As far as I understand the threads synchronize on th_spin_here, so this is guaranteed to be updated. Any other write before this in __kmp_release_queuing_lock is not guaranteed to be synchronized by a weak memory model. This includes th_next_waiting (which triggers the assertion), but also writes by the user application. That's particularly bad because this should be taken care of by the runtime!

In D75001#1922789, @jdoerfert wrote:

In D75001#1891284, @Hahnfeld wrote:

In D75001#1887876, @jdoerfert wrote:

I like this way better. I was hoping we could do it in our cmake only :)

Give others a day or so to comment before your commit but I'm fine with this.

Well, that doesn't really work if openmp-commits is only subscribed on commit. That said, the solution is a bit ugly but I don't have an alternative right now.

What is the problem with openmp-commits here? I got the emails, didn't you?

In D75001#1887876, @jdoerfert wrote:

I like this way better. I was hoping we could do it in our cmake only :)

Give others a day or so to comment before your commit but I'm fine with this.

In D74258#1865532, @JonChesterfield wrote:

Unless there are good reasons to switch, I'd object because it requires newer versions of compilers than many HPC systems have.

I disagree. The use case we would be supporting is:

• building llvm libomptarget from source
• using it with a binary llvm dist, or some other toolchain
• won't download or build a newer toolchain

This should have a RFC on openmp-dev. As far as I understand the linked patch, the aim is to use std::make_unique which is just syntactic sugar IMO. Unless there are good reasons to switch, I'd object because it requires newer versions of compilers than many HPC systems have.

LGTM from the code, I didn't test that the issue is indeed fixed on i386 though

@protze.joachim for 64 bit, we still need at least 5 threads IIRC

As a general comment, I think this patch should be split into at least two separate ones:

1. Load plugins from the same directory as libomptarget.so is found.
2. Quick fail mechanism, although I'm not sure how much this saves? In most cases, we won't have the plugins for a foreign architecture (PPC on x86) anyway, that might only be a concern for accelerators.

In D73249#1835551, @mgorny wrote:

In D73249#1835526, @Hahnfeld wrote:

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/.

If by 'guaranteed' you mean in the current code, then I've tested this patch with it being both true and false.

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

LG

In D70412#1821136, @protze.joachim wrote:

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

This revision was not accepted before being committed!

Can you please also submit a patch to libc++ where we copied this code from?

In D71988#1799064, @jdoerfert wrote:

I would expect that some versioning is needed..

Versioning is fine with me. Versioning the runtime would be even preferable.

Yes, this must not be changed for compatibility reasons. If this really brings a benefit, we need new entry functions and update the compiler(s).

In D71340#1779446, @simoll wrote:

To clarify: this patch is not intended to be committed as-is but gives an overview of the required changes. It's the a patch of a series for the VE backend and includes all the earlier patches.

In D70804#1763443, @bryanpkc wrote:

In D70804#1763289, @ABataev wrote:

Tests are required

The tests for this were added in D30644, but they are currently failing on AArch64 due to D32035. This patch allows those tests to pass again. What further testing should I add?

Random drive-by comments to parts of the patch, I won't review in full.

Testing of openmp is currently very lacking, but I don't think this change will apply cleanly anymore...