LGTM too. Non functional change, clearer code. Thanks

Can this be revived? Changing the enum to a string still sounds good to me

Looks pretty good to me. Thin shim over veo_*.

edit: actually you've already done the clang-format on trunk as I hoped, phabricator mislead me. Apologies for the noise

Sounds good. Thanks for testing it.

In D74361#1927863, @thakis wrote:

This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt

Please take a look, and if it takes some time please revert while you investigate.

Tests made assumptions about alignment which are invalid on arm, failed a buildbot. The tests don't actually care about alignment, so fixed by dropping the , align # part of the patterns.

• Update C codegen test to match output of trunk

Thanks guys. Patched the C codegen test before landing - minor change on trunk in the last week, orthogonal to this.

ping

In D75917#1925617, @sameerds wrote:

Is it really? The scope argument of the IR fence is a target-specific string:
http://llvm.org/docs/LangRef.html#syncscope

The change that I see here is assuming a numerical argument, and also assuming that the numbers used must conform to the OpenCL enum. That would certainly make the builtin quite different from the IR fence.

• undo reformat of existing def

In D74361#1920329, @aaron.ballman wrote:

Aside from the diagnostic wording, I think this LG to me. However, I'd appreciate if @rjmccall would also sign off.

• Amend diagnostic as suggested, clang-format new lines in SemaKinds.td

ping @aaron.ballman - does that look right to you?

In D75917#1916972, @sameerds wrote:

Well, there is a problem: The LangRef says that scopes are target-defined. This change says that scopes are defined by the high-level language and further assumes that OpenCL scopes make sense in all languages. Besides conflicting with the LangRef, this not seem to work with C++, which has no scopes and nor with CUDA or HIP, whose scopes are not represented in any AtomicScopeModel.

In D75917#1916160, @sameerds wrote:

how this builtin fits in with the overall scheme of language-specific and target-specific details of an atomic operation. For example, is this meant only for OpenCL? Does it work with CUDA? Or HIP? What is the behaviour for scope in C++?

This feels familiar.

@jdoerfert this is one of the two intrinsics needed to drop the .ll source from the amdgcn deviceRTL. The other is atomic_inc.

That's less invasive than I feared. Nicely done.

Testing is a sore point. I haven't found an acceptably low boilerplate way to write them for this library yet - should probably take another look.

I thought err_loader_uninitialized_extern says that the variable cannot have external linkage?

• Review comments, add tests for private_extern

In D73979#1857736, @yaxunl wrote:

BTW this is requested by HIP users, who have similar code for CUDA and HIP. They found it surprised that nvcc allows it but hip-clang does not.

• Warn instead of providing a partial implementation

In D75546#1907737, @jdoerfert wrote:

Can we have a print("Locks are not supported in this thread mapping model") instead?

ping

I don't have a documentation reference for this yet. A crude benchmark suggests the frequency is 25MHz on a vega 20, but I'm hoping for a source other than experimentation.

If we say thread == lane of simd, and every lane executes the same instruction with some masked off, then we can 'lock' a thread with respect to the rest of the device if and only if it is the only active one in the warp.

clang-tidy failed because it tried to compile this file for x86, which doesn't work, and because it doesn't like the naming conventions. I can do T r; => T R; and capitalise various names if we like, but the unknown attributes and error on looking for -DAMDGCN will need an infrastructure change to resolve.

• s/warp/wavefront

• Simplify, drop atomicInc impl

At some point in the future we may want to change over to using atomic functions that closer match the C++ ones, and let nvptx implement those in terms of cuda.

I'm finally happy with the semantic checks here. Thanks for the guidance on where to look for the hooks.

• Error on redeclaration with loader_uninit in C

• error on extern variables, minor cleanup

• Reduce scope to trivial default constructed types

I've continued thinking about / experimenting with this. Curiously - X x; and X x{}; are considered distinct by clang. The current patch will only accept the former. I'll add some tests for that.

• Reject initialisers, update doc to recommended string

Fixed the spelling/formatting, added more tests. The C++ case would be improved by warning on int x __attribute__((loader_uninitialised)) = 0 as there are two initializers.

• Address review comments, more test coverage

• Rename attribute, propose some documentation

The above patch composes sensibly with openmp, e.g.

#include <omp.h>
#pragma omp declare target
int data __attribute__((no_zero_initializer));
#pragma omp allocate(data) allocator(omp_pteam_mem_alloc)
#pragma omp end declare target

@data = hidden addrspace(3) global i32 undef, align 4

Issue https://github.com/llvm/llvm-project/issues/159 suggests we missed a file here. Perhaps plugins was previously relying on the compiler default?

In D75285#1897247, @yaxunl wrote:

If users derive a non-const pointer from the const pointer and modify it, doesn't that result in UB? Thanks.

• clang-format

In D74361#1879559, @rjmccall wrote:

This will need to impact static analysis and the AST, I think. Local variables can't be redeclared, but global variables can, so disallowing initializers syntactically when the attribute is present doesn't necessarily stop other declarations from defining the variable. Also, you need to make the attribute mark something as a definition, the same way that e.g. the alias attribute does. Also, this probably ought to disable the default-initialization of non-trivial types in C++, in case that's not already done.

• Rename attribute, add to hasDefiningAttr

• drop the small buffer optimization to simplify the initial implementation

I'd like to rebase this on the current deviceRTL, and add any nvptx/amdgcn specific hooks if necessary. Any objections?

Nice. Thanks!

Cool. Can we reasonably add the reproducible from 44893 to a regression suite, in addition to these IR tests? It's written in C so would need to be under clang's tests.

Thanks! Splitting this out of D71179, which I think ultimately reached consensus, makes the diff much easier to read.

It zero initialises on x86, but perhaps by coincidence rather than guarantee.