IR: Allow vector type in atomic load and store #117625

jofrn · 2024-11-25T20:20:29Z

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

llvmbot · 2024-11-25T20:21:07Z

@llvm/pr-subscribers-llvm-ir

Author: None (jofrn)

Changes

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

Full diff: https://github.com/llvm/llvm-project/pull/117625.diff

2 Files Affected:

(modified) llvm/lib/IR/Verifier.cpp (+6-6)
(modified) llvm/test/Verifier/atomics.ll (+8-7)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 55de486e90e190..83eb1ef0c229a7 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4255,9 +4255,9 @@ void Verifier::visitLoadInst(LoadInst &LI) {
     Check(LI.getOrdering() != AtomicOrdering::Release &&
               LI.getOrdering() != AtomicOrdering::AcquireRelease,
           "Load cannot have Release ordering", &LI);
-    Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy(),
-          "atomic load operand must have integer, pointer, or floating point "
-          "type!",
+    Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy() || ElTy->isVectorTy(),
+          "atomic load operand must have integer, pointer, floating point, "
+          "or vector type!",
           ElTy, &LI);
     checkAtomicMemAccessSize(ElTy, &LI);
   } else {
@@ -4281,9 +4281,9 @@ void Verifier::visitStoreInst(StoreInst &SI) {
     Check(SI.getOrdering() != AtomicOrdering::Acquire &&
               SI.getOrdering() != AtomicOrdering::AcquireRelease,
           "Store cannot have Acquire ordering", &SI);
-    Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy(),
-          "atomic store operand must have integer, pointer, or floating point "
-          "type!",
+    Check(ElTy->isIntOrPtrTy() || ElTy->isFloatingPointTy() || ElTy->isVectorTy(),
+          "atomic store operand must have integer, pointer, floating point, "
+          "or vector type!",
           ElTy, &SI);
     checkAtomicMemAccessSize(ElTy, &SI);
   } else {
diff --git a/llvm/test/Verifier/atomics.ll b/llvm/test/Verifier/atomics.ll
index f835b98b243456..17bf5a0528d738 100644
--- a/llvm/test/Verifier/atomics.ll
+++ b/llvm/test/Verifier/atomics.ll
@@ -1,14 +1,15 @@
 ; RUN: not opt -passes=verify < %s 2>&1 | FileCheck %s
+; CHECK: atomic store operand must have integer, pointer, floating point, or vector type!
+; CHECK: atomic load operand must have integer, pointer, floating point, or vector type!
 
-; CHECK: atomic store operand must have integer, pointer, or floating point type!
-; CHECK: atomic load operand must have integer, pointer, or floating point type!
+%ty = type { i32 };
 
-define void @foo(ptr %P, <1 x i64> %v) {
-  store atomic <1 x i64> %v, ptr %P unordered, align 8
+define void @foo(ptr %P, %ty %v) {
+  store atomic %ty %v, ptr %P unordered, align 8
   ret void
 }
 
-define <1 x i64> @bar(ptr %P) {
-  %v = load atomic <1 x i64>, ptr %P unordered, align 8
-  ret <1 x i64> %v
+define %ty @bar(ptr %P) {
+  %v = load atomic %ty, ptr %P unordered, align 8
+  ret %ty %v
 }

github-actions · 2024-11-25T20:23:47Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jofrn · 2024-11-25T20:53:37Z

#117189 requires this change.

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

llvm/test/Verifier/atomics.ll

jyknight · 2024-12-04T18:34:46Z

I'm a little confused by the steps being taken here.

Can we have this fallback via cast to integer by default on all platforms, at the same time as making it legal? (That's how floats were handled, e.g.). We shouldn't have this work only on some platforms, but not others.

jofrn · 2024-12-04T18:41:33Z

I'm a little confused by the steps being taken here.

Can we have this fallback via cast to integer by default on all platforms, at the same time as making it legal? (That's how floats were handled, e.g.). We shouldn't have this work only on some platforms, but not others.

We can do that. However, the Verifier would still mark it as invalid. The subsequent PR only lowers for X86 ; this PR allows vector types without disabling verifier checks, which is required before enabling for any platform.

jyknight · 2024-12-04T19:18:50Z

I don't understand why do this in multiple steps? Allow vectors in the verifier and update all of the existing implementations of shouldCastAtomic{Load,Story}InIR to cast vectors at the same time?

jofrn · 2024-12-04T20:24:45Z

Verifier checks can be disabled with -disable-verify, which already permits the lowering of atomic load/store with vector type in multiple targets, as such, some are already implemented. Permitting the type in Verifier/IR is different than a single implementation of the lowering.

arsenm · 2024-12-04T20:38:11Z

Can we have this fallback via cast to integer by default on all platforms, at the same time as making it legal? (That's how floats were handled, e.g.).

Disagree, split the IR verifier change from the codegen pieces.

We shouldn't have this work only on some platforms, but not others.

There's a ton of IR that will not work on all platforms with varying failure modes, this case isn't special (e.g. 128-bit atomics)

nikic · 2024-12-04T20:53:02Z

I agree with @jyknight that this needs to come with support for all targets, not just X86. Preferably implemented in a way that does not require defining a hook in every single backend.

We shouldn't have this work only on some platforms, but not others.

There's a ton of IR that will not work on all platforms with varying failure modes, this case isn't special (e.g. 128-bit atomics)

We should always aspire to make generic IR constructs work target-independently. If we have fallen short of that in some area, that's not an excuse to repeat the mistake.

jyknight · 2024-12-04T21:55:52Z

Verifier checks can be disabled with -disable-verify, which already permits the lowering of atomic load/store with vector type in multiple targets

Currently, IR having load/store atomic with a vector type is invalid. If you disable the verifier and provide invalid IR to LLVM, what happens next may be arbitrary -- nobody should be depending on that.

So, I would strongly suggest that the first change be to unconditionally cast the vector to integers -- both in the base class TargetLoweringBase::shouldCastAtomicLoadInIR/shouldCastAtomicStoreInIR and in all the overrides (looks like 3 of those: NVPTX, AMDGPU, SystemZ).

Then, you can make separate follow-up changes for each target to opt out of the cast when possible.

arsenm · 2024-12-04T22:07:51Z

I agree with @jyknight that this needs to come with support for all targets, not just X86. Preferably implemented in a way that does not require defining a hook in every single backend.

Yes, the codegen patch should implement the generic path for all targets. But in terms of patch splitting, that shouldn't be in this PR

We should always aspire to make generic IR constructs work target-independently. If we have fallen short of that in some area, that's not an excuse to repeat the mistake.

There are limits to what is practically implementable for operations that support arbitrary types. ppc_f128, x86_fp80, scalable vectors, and arbitrarily sized atomics are never going to be universally supported.

So, I would strongly suggest that the first change be to unconditionally cast the vector to integers -- both in the base class TargetLoweringBase::shouldCastAtomicLoadInIR/shouldCastAtomicStoreInIR and in all the overrides (looks like 3 of those: NVPTX, AMDGPU, SystemZ).

We really should get rid of these hooks. There's no reason this can't just be handled by ordinary type legalization

jyknight · 2024-12-04T22:24:03Z

Yes, the codegen patch should implement the generic path for all targets. But in terms of patch splitting, that shouldn't be in this PR

I somewhat disagree -- I do think it makes more sense to do this in one commit -- especially since the necessary codegen changes are small. However, that's only a weak preference; if the author wishes to do this in a series of PRs, that's also OK as long they'll all land together.

We really should get rid of these hooks. There's no reason this can't just be handled by ordinary type legalization

Sounds great, but that certainly seems like a separable change. Since these hooks are how it's done today for floats, I don't see a reason to (initially) do something different for vectors.

jofrn · 2024-12-04T23:09:27Z

So, I would strongly suggest that the first change be to unconditionally cast the vector to integers -- both in the base class TargetLoweringBase::shouldCastAtomicLoadInIR/shouldCastAtomicStoreInIR and in all the overrides (looks like 3 of those: NVPTX, AMDGPU, SystemZ).

The change from my end only needs X86 support. If we can get this PR and the next PR in (whether they are together or not), then the internal test will pass. Some of the backend implementations are already implemented too, so we might as well enable it separately.

jyknight · 2024-12-04T23:24:38Z

The change from my end only needs X86 support, and that is all that is implemented at the moment

That is insufficient. The functionality (of passing a vector type to load/store atomic) needs to work for all targets, even if your use-case doesn't require it.

arsenm · 2024-12-04T23:26:39Z

Sounds great, but that certainly seems like a separable change. Since these hooks are how it's done today for floats, I don't see a reason to (initially) do something different for vectors.

I switched AMDGPU to use the DAG legalization in 82bb253. I thought I switched X86 over but that seems to not have landed

The change from my end only needs X86 support.

There's no plus to only doing x86. Just handle in the default implementation of the hook

jofrn · 2024-12-04T23:39:07Z

Regardless of using the hook, backends are still able to implement atomic load/store of vector, so I do not see why we cannot enable the verifier change separately.

nikic

We can land the verifier change separately, but only after a non-x86-specific patch for backend support is available as well.

jofrn · 2024-12-05T12:15:51Z

| We can land the verifier change separately, but only after a non-x86-specific patch for backend support is available as well.
Okay, split #111414 into itself (containing the target independent change) and #118793 (containing the x86-specific change).

jofrn · 2024-12-18T18:36:56Z

Follow-up is here: #120384
@nikic, do you have any other comments?

jyknight · 2024-12-18T23:34:30Z

I'm a little confused as to which PRs are intended to be reviewed now. Is it only the "follow-up" series of 4 PRs you've linked to? Or are some/all of the other 3 PRs you've sent still active?

jofrn · 2024-12-19T02:40:47Z

I'm a little confused as to which PRs are intended to be reviewed now. Is it only the "follow-up" series of 4 PRs you've linked to? Or are some/all of the other 3 PRs you've sent still active?

The follow-up has a set of new PRs associated. #117625, #111414, #118793, and #120316 are now inactive.

jofrn requested review from arsenm, shiltian and Pierre-vh November 25, 2024 20:20

llvmbot added the llvm:ir label Nov 25, 2024

jofrn force-pushed the verifier-atomicvec branch from 4962b17 to af5cac7 Compare November 25, 2024 20:35

[Verifier] Allow vector type in atomic load and store

9b0a33b

Vector types on atomics are assumed to be invalid by the verifier. However, this type can be valid if it is lowered by codegen.

jofrn force-pushed the verifier-atomicvec branch from af5cac7 to 9b0a33b Compare December 1, 2024 19:37

jofrn mentioned this pull request Dec 4, 2024

[X86] Lowering of load atomic vector of float via cast #117189

Open

arsenm changed the title ~~[Verifier] Allow vector type in atomic load and store~~ IR: Allow vector type in atomic load and store Dec 4, 2024

arsenm requested review from nikic, jyknight, RKSimon and efriedma-quic December 4, 2024 17:42

arsenm reviewed Dec 4, 2024

View reviewed changes

llvm/test/Verifier/atomics.ll Show resolved Hide resolved

Update Assembler/atomic test

aba2a03

jofrn mentioned this pull request Dec 5, 2024

[SelectionDAG] Legalize <1 x T> vector types for atomic load #111414

Open

nikic requested changes Dec 5, 2024

View reviewed changes

jofrn mentioned this pull request Dec 5, 2024

[X86] Manage atomic load of fp -> int promotion in DAG #118793

Open

jofrn requested review from nikic and arsenm December 5, 2024 12:22

jofrn mentioned this pull request Dec 17, 2024

[X86] Add atomic vector tests for >1 sizes. #120316

Open

jofrn closed this Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IR: Allow vector type in atomic load and store #117625

IR: Allow vector type in atomic load and store #117625

jofrn commented Nov 25, 2024

llvmbot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

jofrn commented Nov 25, 2024

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024 •

edited

Loading

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024

arsenm commented Dec 4, 2024

nikic commented Dec 4, 2024

jyknight commented Dec 4, 2024

arsenm commented Dec 4, 2024

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024 •

edited

Loading

jyknight commented Dec 4, 2024

arsenm commented Dec 4, 2024

jofrn commented Dec 4, 2024 •

edited

Loading

nikic left a comment

jofrn commented Dec 5, 2024 •

edited

Loading

jofrn commented Dec 18, 2024

jyknight commented Dec 18, 2024

jofrn commented Dec 19, 2024 •

edited

Loading

IR: Allow vector type in atomic load and store #117625

IR: Allow vector type in atomic load and store #117625

Conversation

jofrn commented Nov 25, 2024

llvmbot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 • edited Loading

jofrn commented Nov 25, 2024

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024 • edited Loading

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024

arsenm commented Dec 4, 2024

nikic commented Dec 4, 2024

jyknight commented Dec 4, 2024

arsenm commented Dec 4, 2024

jyknight commented Dec 4, 2024

jofrn commented Dec 4, 2024 • edited Loading

jyknight commented Dec 4, 2024

arsenm commented Dec 4, 2024

jofrn commented Dec 4, 2024 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

jofrn commented Dec 5, 2024 • edited Loading

jofrn commented Dec 18, 2024

jyknight commented Dec 18, 2024

jofrn commented Dec 19, 2024 • edited Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

jofrn commented Dec 4, 2024 •

edited

Loading

jofrn commented Dec 4, 2024 •

edited

Loading

jofrn commented Dec 4, 2024 •

edited

Loading

jofrn commented Dec 5, 2024 •

edited

Loading

jofrn commented Dec 19, 2024 •

edited

Loading