works for me

worked for me

worked for me

Thank you, @ronlieb! I am able to reproduce it. It seems that LLVM_ENABLE_RUNTIMES behavior is quite different from the -DLLVM_ENABLE_PROJECTS="openmp" that I use (so I did not see this issue before). It seems to me that in LLVM_ENABLE_RUNTIMES mode, openmp is configured/built as a standalone project using clang built by the "main" cmake/build step, but for some reason OPENMP_STANDALONE_BUILD is false. I am not sure about the right solution yet. Can you please add openmp to your -DLLVM_ENABLE_PROJECTS="clang;lld" and remove "-DLLVM_ENABLE_RUNTIMES="openmp" as a temporary workaround?

lgtm, thanks for the fix

i forgot to mention that the first problem i encountered with this patch is that elf_common.cpp has difficulties finding its LLVM header files.
i am reporting that issue. For me, the fPIC relocatables might be a downstream issue, still looking into that one.

[AMD Official Use Only]

@carlo.bertolli please review this patch

[AMD Official Use Only]

i can confirm 619.lbm looks better with the patch, peformance recovered,
thanks

i am testing your new patch.
i have no objection if you land what you have.
up to you.

sorry i missed the request for the verbose output, is that still needed from me ?

Hi Joachim, welcome, happy to try again if you come up with another patch.
FI: our benchmark run depends on setting this env-var
export GOMP_CPU_AFFINITY = 0-127

testing results: 128 cores used.

Thanks for putting this together.
if i knew anything about buildbots i would approve it.
but best to defer to an expert.

planning to land this ?

fpspeed base - looks good.

619.lbm looks good base and peak.
Testing all of fpspeed base, should have result in an hour or two.
but i think it is good.

[AMD Official Use Only - Internal Distribution Only]

Thanks for putting up this change, just a few comments.

The AMD AOCC Compiler team reported to me this morning that they are able to reproduce the SPEC CPU performance regressions when the patch is present.

[AMD Public Use]

[AMD Public Use]

[AMD Public Use]

[AMD Public Use]

[AMD Public Use]

[AMD Official Use Only - Internal Distribution Only]

[AMD Official Use Only - Internal Distribution Only]

Originally, yes SPEC CPU was intended to be single core/cpu or rate runs.
with the advent of spec cpu 2017 and the explosion of multicore, spec cpu decided to add openmp to speed benchmarks so that compilers could utilize more cores.
The benchmark have openmp pragmas/directives

i tried using
export LIBOMP_USE_HIDDEN_HELPER_TASK=0 and rebuilding/rerunning spec cpu2017 fpspeed base.

[AMD Official Use Only - Internal Distribution Only]

latest trunk has the assert in question at line 3651
3638 } else {

3639      /* find an available thread slot */
3640      // Don't reassign the zero slot since we need that to only be used by
3641      // initial thread. Slots for hidden helper threads should also be skipped.
3642      if (initial_thread && __kmp_threads[0] == NULL) {
3643        gtid = 0;
3644      } else {
3645        for (gtid = __kmp_hidden_helper_threads_num + 1;
3646             TCR_PTR(__kmp_threads[gtid]) != NULL; gtid++)
3647          ;
3648      }
3649      KA_TRACE(
3650          1, ("__kmp_register_root: found slot in threads array: T#%d\n", gtid));
3651      KMP_ASSERT(gtid < __kmp_threads_capacity);
3652    }

[AMD Official Use Only - Internal Distribution Only]

Post commit issue:
Our downstream testing of our release branch revealed an assertion in kmp_runtime.cpp while compiling our rocFFT application
The rocFFT application does not use openmp offload, rather it uses HIP, and host openmp threads.
When we reverted this patch locally it allowed the application to compile and run succesfullly.
​
root@ixt-sjc2-13:/root/Staging/MathLibs/rocFFT/build/release/clients/staging# cd /root/Staging/MathLibs/rocFFT/build/release/clients/staging; ./rocfft-test --gtest_filter=rocfft_UnitTest.simple_multithread_1D
rocFFT version: 1.0.9.a07759d-dirty
Note: Google Test filter = rocfft_UnitTest.simple_multithread_1D
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from rocfft_UnitTest
[ RUN ] rocfft_UnitTest.simple_multithread_1D
OMP: Error #13: Assertion failure at kmp_runtime.cpp(3691).

LGTM

@jdoerfert acceptable?

added license, and clang-format-ed new file

LGTM

LGTM

lgtm

lgtm

accepted. not sure if there were any outstanding comments from Johannes. your call

The latest patch applied cleanly to our downstream port.
builds fine, tests very nicely as well. All the failing SOLLVE task wait depend tests now pass.

[AMD Official Use Only - Internal Distribution Only]

i applied this patch to our downstream build for amdgcn. it resolves the problem i was seeing with traps on tests with task wait depend tests from sollve.
perhaps rebase it to latest llvm.

LGTM, would like Saiyed and Johannes to review and approve

LGTM, please commit

thanks for the quick response, lgtm.

if its relevant, the llvm was built with gcc , but the test is of course compiled with clang

Thx for the quick fix, this seems to have resolved my issue. LGTM

Johannes: here is a reduce source test case, let me know what else you might need?

Modified patch to assign to IsNew on entry to Device::getOrAllocTgtPtr

LGTM

LGTM

LGTM

I like the general approach,

Thanks folks, publishing now

@grokos any comments here please?
hoping to get this patch in before the 9.0 branch.

thanks for test, LGTM

one follow on question, is there value in adding a testcase for this patch ?

looks like a good change
also consistent with some recent OpenMP language examples committee discussions on this topic.

Superseded by D61313

In D61313#1491568, @hakzsam wrote:

In D61313#1489386, @ronlieb wrote:

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

D60459 doesn't fix the issue.

have you run all the lit tests on this patch ?

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

Added use of LivePhysRegs, happily lifted some code Krzy wrote for Hexagon to compute getLiveRegsAt.

slightly generalized to some physical reg. only look at previous instruction.
The definition is either there, and were all good, or we will bail.

added two MIR tests,and refined logic to properly bail.

Added check for instr match missing, and bail on optimization if so.
I prefer the .ll test we have for the patch now over that of creating an MIR test for this issue.

i think bailing the optimization if not found within some reasonable distance (10 seems to be popular), is a good suggestion. Much better than aborting. thx

The current problem i am trying to resolve in somewhat analogous to hoisting 1/2 of the 64 bit add instruction pair. Although in this particular situation we are actually sinking 1/2 of the instruction pair into a later position within the same block. And yes, i can see how in the future a new machine instruction pass might choose to hoist one of the instructions into a pred BB. I realize i can write additional code to scan a previous block. However i think its better that passes not hoist part of an instruction pair, especially ones such as these. To that end i would rather see my patch assert so that we are forced to deal with such a situation should it arise.
Your example, btw, is a good one for why we should have an IR test for the current problem, rather than an MIR test. An MIR test that runs just before SILoadStoreOptimizer will not detect the affects of a new pass. Whereas the IR test attached to this patch stands a better chance of detecting the issue.

i agree it could happen. Not sure what to do about it here.