So the scheme is: pow is defined in __clang_openmp_math.h to call __kmpc_pow. This lives in libomptarget-nvptx (both bc and static lib) and just calls pow which works because nvcc and Clang in CUDA mode make sure that the call gets routed into libdevice?

Why is it enough to have one counter per warp, what happens if threads within a warp diverge? Before D55773 we had a counter per thread...

In D60578#1467371, @hfinkel wrote:

In D60578#1467324, @ABataev wrote:

So, the test is good or I should put it into some other directory?

@Hahnfeld , libomptarget/deviceRTLs/nvptx/test/parallel works for you for now, correct?

In D60578#1467071, @hfinkel wrote:

In D60578#1466059, @Hahnfeld wrote:

In D60578#1465958, @hfinkel wrote:

In that case these tests need to be marked UNSUPPORTED for versions of Clang that will not pass them. There's infrastructure for that, but it's not applied in the current form of this patch.

Okay. This patch review is not the right place to discuss the libomptarget support for old Clang versions. We should have a separate thread on this subject.

In any case, let's move forward with adding this test in that directory, and then we'll address the infrastructure issue as follow-up work.

So put differently, you're proposing to land this in its current form (which will break for some users, including me) and wait for "somebody" to work on the infrastructure to fix things?

Of course I'm not. I'm proposing that we put the test in the nvptx directory and then address the fact that it should apply to other offloading targets as follow up.

In D60578#1465958, @hfinkel wrote:

What check command do you run to run these nvptx tests?

In D60578#1465420, @hfinkel wrote:

In D60578#1465371, @Hahnfeld wrote:

I think we need to be careful about adding nvptx tests to libomptarget/test: They can be executed using Clang later than 6.0.0, but that version wasn't able to offload to GPUs. Given that the changes are limited to libomptarget-nvptx (because its parallelism is kind of special), I think the new test should go to libomptarget/deviceRTLs/nvptx/test. Just my 2 cents...

I don't see anything in this test that is nvptx specific. Is there something about the semantics that make it specific to nvptx? We need to build of a suite of tests for accelerator offloading in general. We'll have other accelerator backends (e.g., for AMD GPUs), and the offloading tests should apply to them too.

In D60578#1464350, @ABataev wrote:

In D60578#1464304, @hfinkel wrote:

Our general policy is that all commits that can have tests, should have tests. We have OpenMP target tests in libomptarget/test -- and given that you've added tests there yourself, I assume that you know this ;) -- plus tests in libomptarget/deviceRTLs/nvptx/test - although it sounds like this situation can be triggered using portable code, so I'd prefer we add a test in libomptarget/test. Can you please do that?

Sure, if we have a testing infrastructure for this, I'll add the test. Just missed the tests for NVPTX, will definitely add it.

In D60578#1463583, @ABataev wrote:

In D60578#1463545, @jdoerfert wrote:

Why don't we have unit tests here or in the llvm-test suite?

Because this is the library. Do you have an idea how to write the unit tests for it? It can be tested only with the executable tests. I know, that someone worked on the target-based testsuite, but don't know when it is going to be ready.

AFAICS all comments were addressed and this looks good. Please wait a day or so in case I missed something

I guess that change makes sense overall, some comments from reading the code inline. I didn't apply the patch locally so I might be missing some context here...

Most of the changes look pretty straight forward to me, but I currently don't have time to read through the assembly implementation of __kmp_invoke_microtask. I left two comments inline to discuss compressed vs uncompressed encodings for testing OMPT.

The code in cmake/OpenMPTesting.cmake should also handle standalone builds of openmp.

In D59451#1432390, @Hahnfeld wrote:

In D59451#1432389, @krytarowski wrote:

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

So if a developer uses a feature from stdlib, they need to manually link libpthread? I always thought that the C++ standard guarantees you a fully working stdlib out of the box, but might be wrong here.

In D59451#1432389, @krytarowski wrote:

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

In D59451#1432251, @dim wrote:

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

Okay, the test failure is related to my Intel Westmere system (yeah, that's old, I know) where I see:

b = 0x13c0
kBmi12Mask = 0x108

Even the "corrected" code only checked that at least one bit is set :-(

In D45691#1427670, @patricklyster wrote:

Please note this change set broke the following buildbot and has yet to be fixed: http://lab.llvm.org:8011/builders/openmp-clang-ppc64le-linux-rhel/builds/140

In D58858#1426718, @eugenis wrote:

In D58858#1425740, @Hahnfeld wrote:

I'm running that configuration for years and it always worked. Do you have a change at hand that did such compiler change?

Do you run msan lit tests? This is an example where new compiler behavior is tested the same change list, less than a year ago:
https://reviews.llvm.org/rL332761

Hi Doru, I think this patch is not yet in trunk? Additionally I'm seeing the same warnings when compiling with nvcc which doesn't seem addressed here. Should we pass -std=c++11 when compiling the CUDA code as well?

Gentle ping

Ping. As I said I think the current approach with a fixed-size buffer makes sense because anything larger likely won't work anyhow.

In D58858#1425505, @eugenis wrote:

This is expected to break. I'm surprised it did not break earlier - we often do a compiler change and corresponding integration tests in compiler-rt as a single commit, or two consecutive commits.

I think the change is correct, but I don't know enough of the code to feel confident.

The added test is failing for me because I'm building (and testing) compiler-rt with Clang 7 which doesn't instrument BMI, and I'd assume the same with Clang 8 once released. Is that an oversight or is this configuration expected to break?

In D58801#1420179, @hubert.reinterpretcast wrote:

Ping. I am looking for some input on whether we want to pursue additional testing.

Move common ELF description to Inputs/.

In D58837#1416063, @MaskRay wrote:

In D58837#1416042, @Hahnfeld wrote:

I think we could just add a break and handle that case in the generic switch below. But I think you're right, the current patch would break for Hexagon, MIPS and PPC64. I tested with assertions enabled, but maybe there is no test case exercising this code path?

Yes. For this patch using break; should be good enough. There may not be tests for such corrupted binaries.

Replace llvm_unreachables in Object/ELF.cpp by break statements and add test to exercise this code path.

In D58837#1416035, @MaskRay wrote:

getDynamicTagAsString is used by llvm-objdump and it'd be better if llvm-objdump can parse some malformed binaries (with bad .dynsym). If the tag is unknown, falling back to the default label (return "<unknown:>0x"...) may make more sense.

I think this patch is right in also sorting the function names: AFAICS StringMap doesn't provide that guarantee.

In D56286#1411846, @nemanjai wrote:

In D56286#1409384, @hfinkel wrote:

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

I am fairly certain that the STW is the store of the value returned by the function so the scheduler should leave it alone. Perhaps @stefanp can verify this.

I don't care how this is solved, but it needs to be solved! And it's not getting easier with everyone saying something different

In D58787#1413869, @davidxl wrote:

The secondary map 'ProfileData' is needed occasionally (to use cfg hash). doing sorting with std::map is not needed strictly speaking, but it has the nice property of having fixed order.

In D58787#1413840, @lebedev.ri wrote:

Have you considered only imposing the ordering/sorting where it matters,
i.e. i think where FunctionData is used in InstrProfWriter::writeImpl() and InstrProfWriter::writeText()?

In D58385#1410560, @Hahnfeld wrote:

Another approach would be to sort the output and make it independent from reverse-iteration. See D58631 for how this was done for ThinLTO. I'm fine with whatever unbreaks the tests, but I'm not familiar with the code so "fixing" the tests and getting review on this was easier for me.

In D58385#1410546, @lebedev.ri wrote:

In D58385#1410534, @Hahnfeld wrote:

In D58385#1410520, @lebedev.ri wrote:

In D58385#1410516, @Hahnfeld wrote:

Ping, that's now the only test that fails the reverse-iteration bot...

This seems to only shuffle the tests around.
Does this now pass in reverse-iteration config?

Yes, I've tested this locally. The idea for instr-remap.test is to invoke FileCheck three times, checking each function separately.

So then i think i'm missing the point..

If it currently passes without reverse iteration, and does not pass with reverse iteration,
then that means there is some nondeterminism, correct?

In D58385#1410520, @lebedev.ri wrote:

In D58385#1410516, @Hahnfeld wrote:

Ping, that's now the only test that fails the reverse-iteration bot...

This seems to only shuffle the tests around.
Does this now pass in reverse-iteration config?

Ping, that's now the only test that fails the reverse-iteration bot...

Closing after D58631 landed which fixes both the bot and passes in my local build. Thanks @evgeny777!

In D58505#1405772, @krytarowski wrote:

In D58505#1405760, @mgorny wrote:

In D58505#1405719, @Hahnfeld wrote:

Would it make sense to rename the builder? lldb-amd64-ninja-netbsd8 is just not true based on that list. IIRC there are other unified bots, that test a variety of projects?

Apparently Fuchsia has one, and puts it in 'toolchain' category. Maybe we should just name it 'amd64-ninja-netbsd8' or 'netbsd8-amd64-ninja'. Are you maybe aware if rename is something that can be done lightly in zorg, or does it require some extra work?

If we are going to rename it, we probably shall go for netbsd-amd64-tnf.

ninja` is just unimportant detail today it used to be interesting in the time of autoconf vs ninja option (and one worked better than the other), later we might switch to something else and just follow upstream without making non-default options.

netbsd8 soon won't be true once we will upgrade to the -9 branch (or 9.99.x).

tnf specifies hosting by The NetBSD Foundation and a magic shortcut to distinguish it from other potential bots hosted by someone else.

In D58505#1405760, @mgorny wrote:

In D58505#1405719, @Hahnfeld wrote:

Would it make sense to rename the builder? lldb-amd64-ninja-netbsd8 is just not true based on that list. IIRC there are other unified bots, that test a variety of projects?

Apparently Fuchsia has one, and puts it in 'toolchain' category. Maybe we should just name it 'amd64-ninja-netbsd8' or 'netbsd8-amd64-ninja'. Are you maybe aware if rename is something that can be done lightly in zorg, or does it require some extra work?

Would it make sense to rename the builder? lldb-amd64-ninja-netbsd8 is just not true based on that list. IIRC there are other unified bots, that test a variety of projects?

In D58454#1405654, @mgorny wrote:

In D58454#1405643, @Hahnfeld wrote:

In D58454#1405621, @krytarowski wrote:

This broke the NetBSD buildbot: http://lab.llvm.org:8011/builders/lldb-amd64-ninja-netbsd8/builds/19082

This should be fixed in rOMP354572. As a bot owner, could you please make sure that the bot lists the commits to openmp? Otherwise it's hard to detect broken bots without emails.

If I only knew how, I'd have fixed that long ago.

In D58454#1405621, @krytarowski wrote:

This broke the NetBSD buildbot: http://lab.llvm.org:8011/builders/lldb-amd64-ninja-netbsd8/builds/19082

In D51470#1351254, @riccibruno wrote:

This commit made the reverse-iteration bot (http://lab.llvm.org:8011/builders/reverse-iteration) red (red since 6 months now) :

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