Nice. Thanks!

Thanks for the change to uint64_t

• Avoid trip through interface for target_impl

The plumbing looks sane to me. I imagine any bugs or limitations we miss at review will make themselves apparent as more complicated optimisations are included.

Mostly minor comments. You've got a lot of mileage out of x macros. I really like the unit tests generated from the same .def as the source.

In D72525#1817204, @grokos wrote:

but in such problematic code even a 64-bit counter could overflow. Actually the above example is taylor-made to intentionally make the refcount overflow.

Change looks good to me as is. Some notes / questions on the reference counting.

Nice catch, thanks

Great! Thank you

Ah, right. I follow. Some groundwork to be done first then, thanks.

I"d like to understand that use case. Someone compiles libomp from trunk, pastes it over the version from repos, but leaves the rest of the toolchain unchanged. Breakage seems a fair result.

Do we have a consensus / policy on ABI breaks?

I like this, thanks. Very clear.

The premise seems OK, but three copies of a large block of control flow is not so good. Why the duplication?

Big patch but looks like a net decrease in complexity. Please could you clang format the areas phabricator is complaining about?

Thanks!

Looks good, though I haven't built it to check the tests still pass. I take it this is the NFC subset of the previous patch?

Copying this file into the trunk source tree builds for me (using parts of a local aomp build)

• Change to template implementation

• Change to template implementation

It seems to be an openmp restriction that parallel regions can't exit/abort/return etc, so the current path looks fine. Perhaps an assertion on the attribute?

I think next steps from here are:

• get the file building against trunk
• work out what the requirements on ATMI are - I think ROCm and AOMP can use the same version, but haven't tested carefully enough to be sure yet
• clean up the fprintf(stderr...) noise
• try to simplify the cmake searching logic

In D71404#1783988, @jdoerfert wrote:

Let's do that then.

Aside from the inline comment about functions that don't return, this looks good to me. Simple code, heavily tested. Thanks

Apologies for changing the patch post review. The print calls are now back in the omp_ prefixed functions. That seems a better separation of concerns.

• Move print statements back into generic functions

• More explicit cast

Thanks. This is most of the target specific code in libcall, the remaining function is for omp_get_wtime. I'll put up a patch for that shortly.

In D71502#1785967, @ABataev wrote:

In D71502#1785962, @JonChesterfield wrote:

Ping. I'm blocked until this is resolved.

Yes, it works, sorry for the delay.

Ping. I'm blocked until this is resolved.

One would get missing symbols if objects compiled against the previous support.h were linked to ones compiled against the files here, e.g. if one attempted an incremental build of deviceRTL despite the dependency graph changing between revisions.

All passes fine for me, with a debug build using clang 9. Symbols in libraries look fine too.

Thanks. There's something very wrong here - every compilation unit can see the inline definition (else they wouldn't compile), so debug is throwing away the definition without cause to believe there will be an implementation in another TU.

Which build? Debug/release nvcc/clang?

@ABataev wrote:

Reverted in dd8a7fcdd73dd63529b81bf9f72c7529dfe99ec3
Breaks NVPTX tests with unresolved symbols.

Nice feedback, thanks!

Nice cleanup, thanks

That sounds pragmatic. A single header located under common/ will work for nvptx and amdgcn without #ifdef, so lets go with that. We can rearrange the code if necessary when a third target arises or when amdgcn/nvptx wants to implement the atomics differently.

Has anyone actually asked Richard to look at this? He isn't subscribed to the diff and may not be watching openmp-dev.

• rebase

In D71404#1782942, @jdoerfert wrote:

To be honest, I did not follow this so if my response doesn't make sense let me know.

Couldn't we provide these functions in the target_impl.h of amdgcn?

There's an outstanding design point here.

In D71241#1782846, @ABataev wrote:

But I suggest to discuss this with Richard Smith.

In D71404#1782848, @jdoerfert wrote:

template <typename T> T __kmpc_impl_atomic_exchange(T *Ptr, T Val) {
  return atomicExch(Ptr, Val);
}

Enabled by D71404. D71382 is sufficient to compile support.cu if device declarations for malloc, free are provided.

__kmpc_atomic_foo works for me, as does running sed once.

Partially guarding against a user accidentally or incompetently modifying a binary isn't sufficiently useful to justify adding code to lld in my opinion.