LG

I don't see a single test, where you could run into the described problem. And I think this patch should be split into 2 parts: 1 for the first part with the types and another one for extra cost.
Also, I think that the problem with the incorrect cost for X86 is the problem of the X86 TTI implementation rather than the problem of the generic solution.

In D55395#1322431, @rsmith wrote:

In D55395#1322244, @ABataev wrote:

This is wrong, the original implementation is correct and should not be changed.

The original implementation looks pretty clearly wrong to me, but I think this is wrong too. It looks like what was intended here was probably to label the two children as "combiner" and "initializer" respectively. As-is, the "combiner" text on the OMPDeclareReductionDecl line means nothing at all.

This is wrong, the original implementation is correct and should not be changed.

Updated to the latest version.

Reworked according to the latest comments from Eric.

This change is not compatible with NVPTX, it does not support any of the label arithmetics.

LG

In D54342#1313628, @tlwilmar wrote:

In D54342#1313542, @tlwilmar wrote:

In D54342#1313515, @gtbercea wrote:

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

OK, on my internal build these are showing up as weak symbols with nm. But on my LLVM build, not. Will look into the LLVM build to make sure it allows for weak attribute.

I was wrong -- here are the weak symbols that are showing up in libomp:

nm runtime/exports/lin_32e.50.ompt.optional/lib/libomp.so | grep " W "
00000000000a4540 W omp_get_device_num
00000000000a5dd0 W omp_get_device_num_
00000000000a3f20 W omp_get_initial_device
00000000000a57c0 W omp_get_initial_device_
00000000000a3ee0 W omp_get_num_devices@@VERSION
00000000000a5780 W omp_get_num_devices_@@VERSION
00000000000a3f10 W omp_is_initial_device@@VERSION
00000000000a57b0 W omp_is_initial_device_@@VERSION
00000000000a7d20 W ompt_start_tool

So, it looks like weak symbols work better with static libraries. For dynamic, try linking libomptarget first.

This commit conflicts with libomptarget. libomptarget have the same functions and the code may be confused during execution which version of the functions should be executed. This conflict should be resolved somehow. Before that, I think, this patch must be reverted.

LG

Added the new test run independent from the previous one.

In D55017#1312206, @spatel wrote:

Again, we'll need another reviewer to approve this...but let me check my understanding: I think this bug is independent of the fix from D54955? So is it possible to come up with a smaller test case to demonstrate that or would we always hit the previous ReductionRoot problem first?

LG

In D54955#1310191, @spatel wrote:

This makes sense to me, but I'm not very familiar with SLP, so hopefully we can get a 2nd opinion.

By making the reduction root externally used, does that mean that some other pass is now responsible for deleting it (assuming it is not actually externally used) where before SLP would delete it?

LG

LG

In D54764#1304606, @jdenny wrote:

In D54764#1304573, @ABataev wrote:

I'm fine with the patch,

Thanks for the quick reviews. I'll push soon.

the original intention was to handle a possible situation where we may have several debug locations with the same line numbers.

We were afraid of that. If we find we really need this use case, we'll have to find a nicer way to express it. For reference, this use case is discussed under S1 here:

http://lists.llvm.org/pipermail/llvm-dev/2018-May/123258.html

LG

I'm fine with the patch, the original intention was to handle a possible situation where we may have several debug locations with the same line numbers.

LG, mark as NFC