separate CUID patch.

In D85223#2507518, @tra wrote:

I'd propose splitting the patch into two. One with the addition of CUID and the other that changes the way we havdle static vars.
CUID is useful on its own and is relatively uncontroversial.

Externalizing static vars is a more interesting issue and I'm not sure what's the best way to handle it yet. On one hand it is necessary for visibility across host/device, on the other, externalizing all static vars will almost always have negative effect as very few of the static vars actually need this. As already pointed out in the #if 0 section of the patch, ideally we should externalize only the vars that need it. Generally speaking, I do not think we will be able to do that, because with -fgpu-rdc it may be used from the host code in some other TU.

We may need to explicitly annotate such the static variables that need to be visible on both sides and only apply externalization to the variables annotated this way. E.g. require them to be __host__ __device__.

WDYT?

In D85223#2452363, @JonChesterfield wrote:

I concede that making the variables external, and trying to give them unique names, does work around static variables not working. I believe static variables are subjected to more aggressive optimisation than external ones but the effect might not be significant.

This "works" in cuda today because the loader ignores the local annotation when accessing the variable. There is some probably unintended behaviour when multiple static variables have the same name in that the first one wins.

The corresponding change to the hsa loader is trivial. Why is making the symbols external, with the associated complexity in picking non-conflicting names, considered better than changing the loader?

Let's land this fix now. It will take some time to set up the buildbot. I think we already have this issue covered in HIP directed tests, right?

LGTM about r600 changes. Thanks.

can you document this in ClangOffloadBundler.rst ? I think we need a clear description about how clang-offload-bundler knows which file in the .a file belongs to which target.

LGTM. Thanks.

revised by Artem's comments

Pls address Artem's comments. LGTM otherwise.

ping

ping

ping

LGTM. Thanks.

@ABataev Is this patch OK for OpenMP? It is NFC for OpenMP toolchain but affects using clang-offload-bundler as a standalone tool. Thanks.

Thanks for fixing the lit tests. Using regex is the right choice.

In D93258#2453815, @JonChesterfield wrote:

In D93258#2453724, @thakis wrote:

reverted in c9ede6f3367a627baeef78f30d18078af9a4ffca for now

Thanks! Just saw the CI emails come through. I didn't realise code object version was hardcoded across various tests. That's a weird thing to do when it has no effect on the generated IR.

@yaxunl This would never pass the gerrit testing. AMD internal has moved to code object v4 so changing the default to v3 will break internal tests that assume that. The hsa runtime that can load v4 objects hasn't shipped, either in rocm or as source in github, and the backend that generates v4 object files also hasn't shipped, so defaulting to v4 in the trunk front end is a poor choice.

In D92720#2453277, @tra wrote:

In D92720#2448890, @yaxunl wrote:

Output of -E for HIP combined host/device compilation is a plain text. It has C++ comments inserted between preprocessor outputs for host and different GPU arch's. The C++ comments follow the format of clang-offload-bundler bundled text files therefore clang-offload-bundler is able to unbundle it.

OK.

This actually exposed a minor issue. Using something that may legitemately occur in the user source as a separator is rather easy to break.
E.g. there's nothing wrong with the following code, but the bundler will not handle it well if it's used with hip-cpp-output:

const char* s1 = R"foo(
// __CLANG_OFFLOAD_BUNDLE____END__ hip-amdgcn-amd-amdhsa-gfx803
)foo";

It's not a showstopper, but it would be great if the separator would be something that can't be encountered in the preprocessed output.

@JonChesterfield Did this patch pass ePSDB in gerrit? Better do that before committing it to trunk since we don't know if math libs are compatible with this patch. Also you need to fix lit tests.

In D92720#2437621, @tra wrote:

-E by default prints preprocessed output to stdout. CUDA will print preprocessed output from all subcompilations. What does HIP do in this case? Printing out the bundle is probably not what the user will expect.
IMO preprocessed output is frequently used as a debugging tool, so it's important for users to be able to read it. Bundled output is rather cumbersome to deal with. It's possible to manually unbundle it, but the tool is not documented well and it's not particularly suitable for human use.

I think we should preserve the long-established meaning of -E and keep its output as plain text.

revised by Artem's comments

ping

LGTM. Thanks!

Revised by Artem's comments.

Revised by Artem's comments.

Remove unnecessary formatting changes.

Revised by Artem's comments: removing unnecessary output to temporary file, extract forEachBundle.

LGTM. Can we have a test?

In rGcd95338ee3022bffd658e52cd3eb9419b4c218ca#963812, @calewis wrote:

What is the expected behavior in the event of no capture clause? The no capture clause situation was connected to why lambdas behaved this way in the first place.

LGTM. Thanks.

In D92363#2426401, @tra wrote:

While I agree that the default GPU choice is not likely to be correct, or usable, for everyone, but the warning seems to be a half-measure.
If the default is not usable, then it should not be the default. If it's usable, then we don't need a warning.

Having a warning would make sense if it were a part of the plan to transition towards making GPU arch a mandatory option. Is that the case here?
Just a warning is not very useful, IMO. The users would need to specify the GPU in order to silence it, so why not just require it.

ping

In D80450#2426507, @tra wrote:

LGTM.

I'd suggest adding more details on the background of this change to the commit log (point to the comment in the isBetterOverloadCandidate ?) and outline the intention to enable the new way to do overloading after some soak time.

extract lambda as a function

Add -ffix-overload-resolution and fix a regression.

Aaron, Could you please update the bugzilla after the fix is landed? Thanks.

In D80450#2423706, @tra wrote:

SGTM. I'll check how the patch fares on our CUDA code.

If -fgpu-defer-diags is off, keep original behavior.

reopen for fixing the regression

In D80450#2088129, @tra wrote:

In D80450#2087938, @tra wrote:

Reproducer for the regression. https://gist.github.com/Artem-B/183e9cfc28c6b04c1c862c853b5d9575
It's not particularly small, but that's as far as I could get it reduced.

With the patch, an attempt to instantiate ag on line 36 (in the reproducer sources I linked to above) results in ambiguity between two templates on lines 33 and 24 that are in different namespaces.
Previously it picked the template on line 28.

Managed to simplify the reproducer down to this which now reports that a host candidate has been ignored. This may explain why we ended up with the ambiguity when other overloads were present.

template <typename> struct a {};
namespace b {
struct c : a<int> {};
template <typename d> void ag(d);
} // namespace b
template <typename ae>
__attribute__((host)) __attribute__((device)) int ag(a<ae>) {
  ae e;
  ag(e);
}
void f() { ag<b::c>; }

ping

ping

In D71726#2351069, @rjmccall wrote:

Yes, there are no generically available libcalls for atomic float math -- but that's okay -- let LLVM handle transform into a cmpxchg loop when required.

I suspect Yaxun's target cannot provide libcalls at all, which is why he wants to diagnose up-front. But I agree that we should be thinking about this uniformly, and that his target should be diagnosing *all* unsupported atomics.

revised by John's comments. Added target hook and diagnostics for generic atomic operations.

ping