Revision just uploaded annotates all kernels with an integer. That doesn't need to land with the intrinsic but it makes it much easier to drive runtime tests through this code so it's there for now.

• Rebase on main, append metadata to kernels in lower module lds pass

In D125970#3531645, @aaron.ballman wrote:

In D125970#3527685, @JonChesterfield wrote:

If it was adding a calling convention, sure - caution warranted. There's no llvm change here though, an existing CC is exposed to C++. No change to the type system either.

This is adding a user-facing calling convention to Clang and it changes the type system as a result. For example, lambda function pointer conversion operators sometimes are generated for each calling convention so that you can form a function pointer of the correct type (this might not be impacted by your change here); there's a specific number of bits for representing the enumeration of calling conventions and this uses one of those bits, etc.

If it was adding a calling convention, sure - caution warranted. There's no llvm change here though, an existing CC is exposed to C++. No change to the type system either.

A clangd buildbot (https://lab.llvm.org/buildbot/#/builders/131/builds/27770) failed on this with

Unintentionally created this patch against an older version of main and it interacted badly with D124998 on the rebase. Rerunning tests now, and will leave this open for further comments for a little while. Thanks all

• Fix git merge misfires

• Fix git merge misfires

• Rebase on main

Thanks for accepting! I'm interested to learn more about how the calling conv works, e.g. if parts of it are implemented in clang and parts of it patched on the fly by opt, but that's downstream of easy access to writing C tests that use it.

Added a codegen test for arg passing. It establishes that most arguments are left alone, but structs passed by value are handled as an addrspace(4) byref. Letting opt -O2 run annotated some argument pointers as being in addrspace(1) which I think is wrong.

• Add O0 arg passing codegen test

OK, so that's a different thing. CUDA/HIP has a bunch of rules about implicitly tagging things with addrspace(1) at the call boundary. I don't think any of that magic should exist for C or C++, the developer gets to spell out the address space stuff they want explicitly, and if they want to link it against CUDA/HIP/OpenMP code they get to look up the rules. In particular, persuading clang to do the extra argument mangling stuff will get in the way of using this to create test cases.

In D125970#3525985, @yaxunl wrote:

need a codegen test to make sure amdgpu_kernel ABI is used in C/C++ for functions with this attribute

Looks about as expected, modulo Johannes' comment above. Could we do the getenv call once, instead of per-launch? Somewhere in the massive class constructor perhaps.

Drop the caching of the value and pass in function instead of module. No functional change to previous diff, simpler code.

• Module->function, skip caching the attribute

Didn't notice the hasFnAttribute request comment, change made. Anything else?

• rebase, and getFnAttr -> hasFnAttr

Nice! I like the strategy.

LGTM, thanks! If it fails CI we might want to submit the tests and the code change separately, but I'm fine with trying it as one block in the first instance.

Thanks! Good fix

Thanks for the extra test lines!

Editing the tests in place to check for new driver properties would mean we lose coverage for the old driver and get a lot of churn if we need to back out the change.

Couple of nits above but basically I'm convinced. The gnarly part of binary formats is string tables and I'm delighted that part of MC was readily reusable. Wrapping the string table in different bytes to align with the elf format may still be a good idea but it's not an obvious correctness hazard.

Nice, thanks!

Extra dependency edge is great, thanks! Is LIBOMPTARGET_TESTED_PLUGINS new? It looks unused in this patch

I think we need tests for coff and elf for the MC part, should be existing ones for the other flags. Ideal fit for the openmp sectiy, good find. Inclined to ignore the formatting warnings on old code.

Ron thought @bcahoon might be interested, adding as reviewer

@arsenm there's a bunch of stuff to do that builds on this. What further changes would you like?

Added some reviewers. I'd much prefer this used an existing binary format, DIY is prone to errors and maintenance hassles down the road. Don't care as much about which format as about it being one with an existing, tested implementation and ideally existing inspection tools.

In D122091#3404987, @hsmhsm wrote:

What happens to the case where the entry point function makes call(s) to device functions, but none of them use lds?

Nice, thanks. Wonder if we want protected visibility as well.

Sounds good to me too, thanks!

• Drop string bool parameter and unreliable test

• invert sense of attribute

In D118493#3401448, @tstellar wrote:

The rule that is broken is "standard RPATHs" Fedora installs libomp to /usr/lib64.

...

I think what we'll do in Fedora is just add -fno-openmp-implicit-rpath to the default CFLAGS so we can get the old behavior for official RPMs.

@estewart08 Fedora are rejecting some uses of rpath (and probably runpath). I can't find a corresponding page for redhat. This may become a problem for our aomp/rocm builds.

It lets applications run with the libomp and libomptarget built with the toolchain. For users, they don't have to be root. For compiler devs, we test our work instead of whatever the distro had installed.

The current behaviour is to allocate an instance of llvm.amdgcn.module.lds.t at address zero in every kernel in the IR module if the global llvm.amdgcn.module.lds is present. That instance only needs to be allocated in kernels which subsequently call a function which uses it.

The other passes do know how much is allocated. I don't want to go digging around the IR looking for the llvm.donothing call, which iirc is removed before instruction selection

@ronlieb this is the patch I'd like to run through the internal CI

Thanks!

Nice, thanks!

• apply refactor from D115980, removes typo-prone part

This can be picked up and simplified using D121499

Given the header proposed in D121499, OpenMPMath.cpp can be replaced with:

Sounds good to me.

@ronlieb this probably means our CI needs to do different things

We've got quite a lot of debt in this area and seem at risk of taking on more. Ideally the cuda and hip and openmp headers would be closer to a single header containing:

OK, I think I follow.

I'm ooo for a couple of days, @tianshilei1992 could you take a look?

Found one error in the existing code but can believe there are more. We could split this patch into two, one replacing the DIY LockGuard with std::lock_guard and clang-format the thing, then a second replacing manual locking with uses of lock guard. Would be the same behaviour as this patch but make it easier to find the change in behaviour by inspection.

Only comments on the helper type for now. I like the idea of a generic wrap-this-thing-in-mutex helper as a path to stepping on the remaining race conditions.

Great, thanks!

Nice, thanks!

Thanks! I get the opposite, building locally with clang sometimes breaks gcc buildbots.

Broke a couple of buildbots, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/7799

Thanks! Seems a good thing to add to the offloading test runner, preferably in a separate change to avoid reverting this in case of unforeseen problems

LG, thanks for including the test

Thanks!

This is asserting somewhere in clang-linker-wrapper, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/6939

I'm not confident it's only attributes that we rely on mlink-builtin-bitcode patching up, that could be poor phrasing on my part.

Nvm, saw the commit message

Cross compilers are a hazard here. I'd expect there to be a fairly long list of magic flags you need to pass to clang to get it to find the right libraries. Can you add fno-openmp-implicit-rpath to that list instead?

That's somewhat interesting, @arsenm the 4000 series APUs used to identify as gfx902 iirc - does the new name for existing hardware make sense to you?

Thanks!

Looks reasonable to me. This is looking at 80 bit floating point which x86 can do (in some modes at least, this passes mlong-double-80 ). Long double elsewhere tends to be 64 or 128 bit.

Thanks!

Can we drop an xfail in an existing test as part of this patch?

I've read this closely and can't think of anywhere else that needs to be patched. Thanks!

@ronlieb and I think reverting this is sufficient to get us a buildbot running again, going to try it. f52927c122edd7d237eb4adafd31eba93342923c

@ronlieb @jdoerfert This was hoped to resolve the breakage from https://lab.llvm.org/buildbot/#/builders/193/builds/5772 which unfortunately has five changes in it,