IMO the current logic makes sense for the end user, prioritizing the environment guess over default paths. I think the tests should be fixed using --cuda-path-ignore-env, I remember I added this parameter to fix the failing tests when I wrote the code.

In D51875#1229565, @ABataev wrote:

In D51875#1229545, @Hahnfeld wrote:

In D51875#1229536, @ABataev wrote:

I already described it - it breaks the compatibility with other outlined regions and breaks the whole design of the OpenMP implementation.

First that's a general statement without any explanation. Second I'm not asking about the scratchpad pointer solution in ibm-devel but rather why we can't pass RequiresDataSharing = true to __kmpc_spmd_kernel_init. Which will give us the data sharing in existing buffers.

First, stop talking like this. I don't owe you anything.

In D51875#1229536, @ABataev wrote:

In D51875#1229517, @Hahnfeld wrote:

In D51875#1229506, @ABataev wrote:

In D51875#1229502, @Hahnfeld wrote:

In D51875#1229496, @ABataev wrote:

In D51875#1229491, @Hahnfeld wrote:

I really, really dislike adding even more global buffers. 4096 * 32 * 56 are another 7MiB that are not usable for applications. What's wrong with using the existing ones?

Can you upload the CodeGen patch for reductions somewhere? I thought we need a global scratchpad buffer that is adressable for all teams?

I really, really dislike an implementation in ibm-devel, the scratchpad solution will never be added to the trunk. The existing ones cannot be reused, as they are allocated only if the full runtime is used.

What's the overhead of initializing it? The whole libomptarget-nvptx is already a pretty much mess, see my thread on openmp-dev.

It is not the runtime issue, it is the problem with the compiler itself. It breaks compatibility with the other outlined regions and, thus, it cannot be committed to trunk.

Can you please describe the problems? Again, maybe posting the patch may help.

I already described it - it breaks the compatibility with other outlined regions and breaks the whole design of the OpenMP implementation.

In D51875#1229506, @ABataev wrote:

In D51875#1229502, @Hahnfeld wrote:

In D51875#1229496, @ABataev wrote:

In D51875#1229491, @Hahnfeld wrote:

I really, really dislike adding even more global buffers. 4096 * 32 * 56 are another 7MiB that are not usable for applications. What's wrong with using the existing ones?

Can you upload the CodeGen patch for reductions somewhere? I thought we need a global scratchpad buffer that is adressable for all teams?

I really, really dislike an implementation in ibm-devel, the scratchpad solution will never be added to the trunk. The existing ones cannot be reused, as they are allocated only if the full runtime is used.

What's the overhead of initializing it? The whole libomptarget-nvptx is already a pretty much mess, see my thread on openmp-dev.

It is not the runtime issue, it is the problem with the compiler itself. It breaks compatibility with the other outlined regions and, thus, it cannot be committed to trunk.

In D51875#1229496, @ABataev wrote:

In D51875#1229491, @Hahnfeld wrote:

I really, really dislike adding even more global buffers. 4096 * 32 * 56 are another 7MiB that are not usable for applications. What's wrong with using the existing ones?

Can you upload the CodeGen patch for reductions somewhere? I thought we need a global scratchpad buffer that is adressable for all teams?

I really, really dislike an implementation in ibm-devel, the scratchpad solution will never be added to the trunk. The existing ones cannot be reused, as they are allocated only if the full runtime is used.

I really, really dislike adding even more global buffers. 4096 * 32 * 56 are another 7MiB that are not usable for applications. What's wrong with using the existing ones?

Split testing infrastructure from bugfix.

In D51107#1225966, @grokos wrote:

In D51107#1224877, @ABataev wrote:

In D51107#1224291, @Hahnfeld wrote:

I think it's more consistent to handle all data mapping in a single loop (in target_data_begin) as discussed before.

I think this is a different problem that should be solved separately. It requires a redesign of the existing solution, while I'm just trying to implement a new feature.

I think I agree with Alexey here. I favor letting support for lambda captures in and later on we can redesign the way libomptarget handles mappings. Is there any objection?

In D51107#1223432, @ABataev wrote:

In D51107#1223428, @Hahnfeld wrote:

In D51107#1211075, @ABataev wrote:

BTW, the main problem here is that there is no mapping between host pointer and device pointer for the firstprivates, just like for the mapped data. We just need to rework the way we allocate the memory for the firstprivate to make MEMBER_OF to work.

I don't see this path taken in the latest patch. Is there a major drawback of doing this?

I did it using new tgtArgsPositions, which handles mappings between the firstprivates and the corresponding target pointers.

LG, but please revise the summary to say that the variable was unused. I think that's enough of motivation, no need to refer to "potential problems" or other patches.

In D51623#1223437, @sdmitriev wrote:

Not related to this patch, but I think there is one more potentially incorrect use of containers in RTLsTy::RegisterLib(). This function may increase size of the global vector Devices when it initializes a new RTL that has not been yet used (see rtl.cpp line 219). This may result in memory reallocation and thus all pointers to the Devices elements that are stored in RTLs (see rtl.cpp line 228) would become invalid. I am not sure if these pointers are really used by RTLs now, but if yes, than it is a problem. RTLsTy::RegisterLib may be called more than once and each call may potentially initialize new RTLs.

In D51107#1211075, @ABataev wrote:

BTW, the main problem here is that there is no mapping between host pointer and device pointer for the firstprivates, just like for the mapped data. We just need to rework the way we allocate the memory for the firstprivate to make MEMBER_OF to work.

In D51623#1223117, @grokos wrote:

Good catch, thanks for the fix!

Sorry for the delay!

Not needed anymore after the reverts in rC341115 and rC341118, right?

Not needed anymore after the reverts in rC341115 and rC341118, right? Maybe we should revive the test to make sure we don't break this in the future?

In D50845#1219865, @gtbercea wrote:

In D50845#1219859, @Hahnfeld wrote:

removing InitializePredefinedAuxMacros and the new test completely should do.

Yep they also contain D51312 in case you're rolling back individual commits.

In D50845#1219853, @tra wrote:

In D50845#1219819, @Hahnfeld wrote:

Ok, the top preprocessor condition for that function is #ifndef __SSE2_MATH__ - the exact same macro that was part of the motivation. Can you please test compiling a simple C file (including math.h) with -mno-sse? My guess would be that this is broken as well.
If yes I'm fine with reverting because I need to teach Clang to allow anonymous unions in type specifiers to make that weird system header work with this patch.

It compiles fine. The code that causes the problem is also conditional on !defined __NO_MATH_INLINES and it's always defined for X86, so compilation only breaks for when we compile for NVPTX.

In D50845#1219746, @tra wrote:

In our case the headers from a relatively old glibc and compiler errors out on this:

/* This function is used in the `isfinite' macro.  */
__MATH_INLINE int
__NTH (__finite (double __x))
{
  return (__extension__
	  (((((union { double __d; int __i[2]; }) {__d: __x}).__i[1]
	     | 0x800fffffu) + 1) >> 31));
}

expanded to this:

extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) int
 __finite (double __x) throw ()
{
  return (__extension__
   (((((union { double __d; int __i[2]; }) {__d: __x}).__i[1]
      | 0x800fffffu) + 1) >> 31));
}

The error:

.../include/bits/mathinline.h:945:9: error: '(anonymous union at .../include/bits/mathinline.h:945:9)' cannot be defined in a type specifier
          (((((union { double __d; int __i[2]; }) {__d: __x}).__i[1]
               ^
.../include/bits/mathinline.h:945:55: error: member reference base type 'void' is not a structure or union
          (((((union { double __d; int __i[2]; }) {__d: __x}).__i[1]
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

Also, whatever macros we generate do not prevent headers from using x86 inline assembly. I see quite a few inline asm code in preprocessed output. The headers are from libc ~2.19.

In D50845#1219797, @tra wrote:

I've sent out D51501. It unbreaks CUDA compilation and keeps OpenMP unchanged.

In D50845#1219726, @gtbercea wrote:

In general, it looks like this patch leads to some host macros having to be defined again for the auxiliary triple case. It is not clear to me how to exhaustively identify the missing macros, so far it's been just trial and error.

Do you have invocations or headers that don't work? The problem is that the previous code defined all macros unconditionally, so it will afterwards be hard to find the necessary macros...

Please also update the test.

In D51378#1218184, @RaviNarayanaswamy wrote:

We should just go with generating an error if the DeclareTargetNestingLevel is not 0 at the end of compilation unit.
Hard to detect if user accidentally forgot to have end declare in header file and had it in the include file or it was intentional.

Should be fixed in rL340767

LG

Yes, see D51226 which will replace the varargs function by a macro.

The call must not be put into conditional compilation. As discussed in D50774 there are two options:

1. Guard the variable definition and assignment (the first line) by preprocessor guards.
2. Add a (void) *_status; to silence the warning.

LGTM. Can you add a comment to InitializePredefinedAuxMacros explaining that the macro is used in gnu/stubs.h and add a check to the test?

LGTM. One very minor comment inline, I leave it up to you whether you want to make that change - it's more important that we unbreak the build.

Yeah, sorry for break. Alex is currently favoring to rewrite the varargs functions into macros: D51226

No, that's completely different from what I asked.

In D50939#1211391, @ABataev wrote:

In D50939#1211333, @Hahnfeld wrote:

It doesn't work for C++ yet because the LLVM function names are already mangled, so we need to do the same when checking in Sema. I think the tricky thing will be templates: Because we don't know which specialization will eventually be emitted we might need to check all of them if a template is referenced in device code. Thoughts?

I expected a lot of problems with C++ and I don't think we will be able to resolve this, especially with the templates. We need to try some other solutions.

In D50845#1212643, @tra wrote:

Please keep an eye on CUDA buildbot http://lab.llvm.org:8011/builders/clang-cuda-build.
It runs fair amount of tests with libc++ and handful of libstdc++ versions and may a canary if these changes break something.

Based on libc++ I guessed some more macros that may be needed on macOS and Windows. As I can't test myself if somebody else could report if this change is regressing CUDA support on these platforms.

As long as we don't need to change back to the current way of doing things next week, I think I'm also fine with the macro if you think it's better. Please address the inline comment about do { ... } while (0) before committing.

@ABataev what's wrong with cstdarg?

In D51226#1212699, @AlexEichenberger wrote:

In D51226#1212685, @Hahnfeld wrote:

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I am reimplementing this because it needs an include that may be fragile on some platforms (D51219)

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?