LG if that fixes the problem. For the record llvm/runtimes/ does the same.

In D58333#1400699, @phosek wrote:

This is an alternative to D58331 which is an attempt at resolving the breakage introduced in D58013. I think this is a better solution than D58331.

@phosek Sorry for the breakage. I guess there are just too many configurations for non-trivial changes to the build system :-/

I needed to push rCRT354231 to fix the sanitizer bots. Let me know if this change is appropriate or want me to revert. In that case we need to run another round of clobber builds to fix CMake configuration (needed because the patch changes the source directory for ExternalProject_Add).

Revert changes to CMAKE_THREAD_LIBS_INIT.

I had to revert this in r354153 because it breaks sanitizer-x86_64-linux:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/19915

Cleaunup CustomLibcxx/CMakeLists.txt and build custom libcxx without exceptions.

@phosek Excellent idea. I tried to implement what you described and it seems to work great! The only minor disadvantage is that we now need to disable the tests if COMPILER_RT_LIBCXXABI_PATH is not found...

Rebase, apply the same treatment to TSan, and rename option to SANITIZER_TEST_CXX.

In D58014#1392222, @phosek wrote:

Wouldn't it be easiest to link the instrumented libc++ statically and avoid the rpath issues altogether?

Excellent!

As said in D57851, this will not handle Clang and only work for GCC.

In D57851#1387935, @twoh wrote:

@kkwli0 Thanks for letting me know! But wouldn't https://reviews.llvm.org/D57848 always fall to "else" case added if the compiler is not GCC?

I think it's a good idea to have a proper RFC on cfe-dev about how to handle runtime libraries in case they are already installed in system directory. More pointers on historic discussions below.

In D55725#1369528, @jdenny wrote:

In D55725#1369006, @Hahnfeld wrote:

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

LIBRARY_PATH during compilation has no effect for me.

First, -v reveals that -L/home/jdenny/llvm/bin/../lib is before -L/home/jdenny/llvm/lib, which is added by LIBRARY_PATH, which is thus redundant here.

I think this patch would effectively lose the ability to use a custom standalone build of the OpenMP runtime: When hacking on the runtime it's pretty convenient to only build and install the openmp repository and use environment variables to switch between different versions. When putting libomp.so into lib/clang/9.0.0/lib/linux/x86_64 [1] this path will be very first one that Clang instructs the linker to look at. As a result the "custom" location that I specify in LIBRARY_PATH won't be honored and I'll always end up using the version installed next to the compiler.

I don't know anything about GlobalISel implementation. The patch seems to do the right thing, but I'll defer to somebody who knows the code.

Yeah, sorry about that: That message was before I got the chance to really look into the generated assembly. I posted my analysis in the referenced bug you reported (https://bugs.llvm.org/show_bug.cgi?id=40131#c6) and frameaddress is also missing from GlobalISel which is why some tests are still failing. Fortunately there is https://reviews.llvm.org/D56266 which should restore -fno-experimental-isel and turn all tests green again until somebody has time to fully address all issues related to GlobalISel.

I think you should also revert the other changes from rL349512.

Did you also implement frameaddress in GlobalISel (see llvm.org/PR40131)? Otherwise this won't make all tests work and it would be better to "fix" -fno-experimental-isel.

Thanks for the explanation, that sounds useful.

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.

I think there was a discussion in the past whether it makes sense to allow building for an older version of the standard, given that the library guarantees backwards compatibility. Your findings once again show that nobody is even compile-testing the old code, so I think it would be best to just remove all the #ifdefs and code that is not needed anymore. For everything else there's a version control system that you can ask about old versions of the code.

I think there are some misunderstandings here, or at least I understand things differently than @tra is describing them: AFAICS this change is NOT about replacing nvcc by clang in any CMake project (be that the OpenMP runtime or any other project outside of LLVM). The scope of the second issue is that there is a CMake project (namely openmp) which uses FindCuda.cmake to detect the CUDA installation and passes the resulting path to --cuda-path. I wrote that detection and I still think it's correct because that is a way to force Clang to use a given CUDA install location. If FindCuda.cmake returns a wrong path (/usr instead of /usr/lib/cuda) or the shim package behind /usr/lib/cuda is incomplete, then this needs to be fixed and not workarounded in Clang.

Ah, this packaging style is a never ending source of fun... Adding isUbuntu() seems fine to solve the first issue.

As said on D53250 I think this is the right way to fix these tests: We already do the same for -stdlib=platform and -rtlib=platform.

If I read that patch correctly, this will render -fuse-ld with non-absolute paths useless if a toolchain has DefaultLinker != "ld". I don't think that's what we want to do if the user explicitly sets a different linker.

In D53141#1262019, @ABataev wrote:

In D53141#1261996, @Hahnfeld wrote:

I guess this will break the case when the DataSize passed to __kmpc_data_sharing_push_stack() needs additional alignment: With this change it is handled in data_sharing_push_stack_common() but __kmpc_data_sharing_push_stack() will determine PushSize without the adjustment and do the final pointer arithmetic.

Why DataSize might require an additional alignment? The DataSize must be already aligned.

I guess this will break the case when the DataSize passed to __kmpc_data_sharing_push_stack() needs additional alignment: With this change it is handled in data_sharing_push_stack_common() but __kmpc_data_sharing_push_stack() will determine PushSize without the adjustment and do the final pointer arithmetic.

Ping

Out of interest: Is this fixing a particular issue?

In D52733#1252966, @ABataev wrote:

In D52733#1252963, @Hahnfeld wrote:

In D52733#1251421, @ABataev wrote:

It might lead to increased register pressure, isn't it? Currently, I'm trying to emit the code that can be optimized out and, thus, may decrease the register pressure. That's why I tried to reduce the number of the runtime checks.

You are right, it's increasing register usage but I think it shouldn't: The generated code is always checking __kmpc_is_spmd_exec_mode first. So if LLVM would be able to optimize this out in SPMD mode, __kmpc_parallel_level should never be called.

I guess this doesn't work because it's illegal to hoist the load of execution_param across a barrier?

Even if we will be able to reduce register usage for SPMD, it still going to be high for non-SPMD constructs. The optimizer is not able to understand that it is in parallel region or not during the compilation phase

In D52733#1251421, @ABataev wrote:

It might lead to increased register pressure, isn't it? Currently, I'm trying to emit the code that can be optimized out and, thus, may decrease the register pressure. That's why I tried to reduce the number of the runtime checks.

In D52701#1250645, @grokos wrote:

Looks good. I have no idea why this option was left there...