LGTM

In D144759#4179328, @arsenm wrote:

Still don't understand why this isn't just a generic / default implementation

Simplified patch to avoid the optimization on any atomicrmw with a release semantics. A monotonic or acquire does not require a fence or cache flush.

Increasing the limit may result in more spilling in other cases. In general a good performance testing is needed to reason if this is beneficial.

Just discussed it with Tony. This seems somewhat problematic as exploiting a general lack of other atomic optimizations and that we cannot really reorder a fence. But then we only really need it for relaxed atomic and can safely do without a fence for a relaxed or acquire atomic. So let's keep it simple and only do the optimization if there is no release semantics on the atomicrmw. I will update the patch.

OK, let's be on a safe side. https://www.hpl.hp.com/techreports/2012/HPL-2012-68.pdf tells than a release fence is needed for load ordering if rmw is release or stronger. Legalizer does not do it just by itself, although the only noticeable difference in codegen is with seq_cst, which looks reasonable.

LGTM, but please add a comment why are you checking predecessors here. Something like: "Live-ins of a successor are the same as live-outs of a predecessor, but subreg mask may be different for different predecessors."

I feel like it's time to ask Tony.

LGTM, please give Matt a chance to review.

LGTM, modulo Matt's comment.

PSDB passed.

In D142507#4127505, @b-sumner wrote:

My current understanding is the c-p will go into already forked clang-16, but not to rocm 5.4. So rocm device-libs will be accompanied by the older clang-16 w/o this and stay compatible. Someone building from scratch will use latest clang-16 and staging device-libs with this change. Do you think this will work?

I wouldn't recommend it. I would patch whatever device libs are being built in association with clang-16, not staging. Staging device libs is only appropriate for the staging compiler. A hash of device libs from around the time that clang-16 stable released would probably be safe.

In D142507#4127421, @b-sumner wrote:

I have no objection to backporting this, but it may need to be accompanied with a device-libs patch, and I don't know where that patch would be checked in. The ROCm-Device-Libs in github certainly doesn't have a "clang-16" branch.

In D142507#4127374, @aaronmondal wrote:

I think unless conflicts arise creating an issue similar to this https://github.com/llvm/llvm-project/issues/60600 with the cherry-pick line set to this commit should be enough. (See also https://llvm.org/docs/GitHub.html).

In D142507#4127275, @aaronmondal wrote:

I cannot say there was much choice. The only real choice was to postpone the split and magnify the problem in the future. As for the ifdefs, this might be possible in the device-libs but I do not see how to do it the Builtins.def.

Hmm maybe ifdefs in the device libs would also just delay the issue. Maybe it really is best to pull this change into Clang 16 and accept the fact that it's an unfortunate situation, but at least give users with very recent hardware the option to use a regular Clang to build ROCm. Realistically, those actually upgrading to Clang 16 early will also be those upgrading to ROCm5.5 early and likely also be those most likely to have 7900 GPUs.

Somehow, telling users "if you have a new GPU you need new Clang + ROCm" and "if you want new ROCm for your old GPU you need to also upgrade Clang" sounds better to me than telling them "if you have a new GPU you are SOL unless you use binary releases or build the amd-llvm-fork" 😅

In D142507#4127167, @aaronmondal wrote:

Well, I can already feel the pain that distro maintainers having to build the next ROCm releases 😅

I wonder what the better course of action is here:

1. Port this patch to Clang 16 so that users with new hardware will be able to build ROCm 5.5, but make it impossible to build ROCm 5.4 and older with clang 16.
2. Don't port this patch and have a ~6 months gap during which users with the 7900 GPUs won't be able to build ROCm with a stable Clang version, requiring distro maintainers to use several toolchains and source-based distro users to use differentl compatibility patches for different ROCm releases. So basically when 8900 GPUs are announced, clang would support ROCm for 7900 GPUs 😅

Would there be a way to retain at least *some* backwards compatibility or version interoperability? For instance, via an #ifdef CLANG_VERSION_MAJOR in the device libs and an #ifdef INCOMPATIBLE_AMDGPU_INSTS in Clang?

This would obviously very ugly, but it still seems better to me than locking out users (and more likely, ROCm contributors) from using 7900 GPUs if they are unable to build Clang themselves. Users already complain about how hard it is to build ROCm, and they also complain about the frequent breaking changes Clang. I'm very much in favor of moving fast, but I'm worried that complete disregard for backwards compatibility like this with no clear upgrade path or fallback mechanism could cause a lot of frustration for users and distro maintainers.

Maybe there is some other, prettier way to solve this? 🥹

In D142507#4126864, @aaronmondal wrote:

It shall be complimented by the device-lib change in the corresponding release, so it is not that simple.

@rampitec I'm not sure I understand. Does this mean that this is breaking in a way that Clang 17 won't be able to build ROCm 5.4?

I thought it was like "we need D142507 to build device-libs after 8dc779e" and for older device libs we just fall back to some older behavior.

Renumbered registers.

In D142507#4125940, @aaronmondal wrote:

Would it be possible to backport this to Clang 16?

If https://github.com/RadeonOpenCompute/ROCm-Device-Libs/commit/8dc779e19cbf2ccfd3307b60f7db57cf4203a5be makes it into ROCm 5.5 no distro would be able to build it with "vanilla" Clang 16, potentially causing pain for users that try to build ROCm 5.5 with a Clang from a package manager (a realistic scenario, considering that one may want to invest 5 min to build ROCm but not 40 min to build Clang). ROCm 5.5 will be the first release to officially support the 7900XT and 7900XTX, so not having this potentially causes issues for users with recent AMD hardware. (See https://github.com/RadeonOpenCompute/ROCm/issues/1880 for extensive, related discussion).

@jhuber6 This wouldn't exactly "solve" https://github.com/llvm/llvm-project/issues/60660, but I think this could also be a workaround (with potentially better user experience), as allowing users build ROCm with regular Clang 16 prevents that deadlock where we can't build ROCm anymore. This is entirely based on speculation that ROCm 5.5 won't introduce other breakages before its release though, so I'd totally understand if this is not a satisfactory solution.

Added testcase.

In D143934#4123865, @arsenm wrote:

testcase?

LGTM

In D143603#4113800, @arsenm wrote:

It's even safe for system scope?

LGTM, thanks!

Split the cleanup NFCI.

Rebased.

In D142493#4077878, @foad wrote:

[AMDGPU] Remove dot1 and dot5 features from clang for gfx11

"dot1 and dot6"?

LGTM

Moved initialization.

LGTM

Added debug output.

ping

Testing showed negative performance impact. D141876 is the way to go.