IR/Verifier: Allow vector type in atomic load and store

[jofrn]

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

---

Stack:

• [AtomicExpand] Add bitcasts when expanding load atomic vector #120716
• [X86] load atomic vector tests for combine #120640
• [SelectionDAG][X86] Widen <2 x T> vector types for atomic load #120598
• [X86] Add atomic vector tests for unaligned >1 sizes. #120387
• [X86] Manage atomic load of fp -> int promotion in DAG #120386
• [SelectionDAG] Legalize <1 x T> vector types for atomic load #120385
• IR/Verifier: Allow vector type in atomic load and store #120384 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

[llvmbot]

@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.

---

Stack:

• #120387
• #120386
• #120385
• #120384 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

---

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

4 Files Affected:

• (modified) llvm/docs/LangRef.rst (+4-4)
• (modified) llvm/lib/IR/Verifier.cpp (+8-6)
• (modified) llvm/test/Assembler/atomic.ll (+9)
• (modified) llvm/test/Verifier/atomics.ll (+8-7)

diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 7e01331b20c570..eb856044b23c3f 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -10956,8 +10956,8 @@ If the ``load`` is marked as ``atomic``, it takes an extra :ref:`ordering
 <ordering>` and optional ``syncscope("<target-scope>")`` argument. The
 ``release`` and ``acq_rel`` orderings are not valid on ``load`` instructions.
 Atomic loads produce :ref:`defined <memmodel>` results when they may see
-multiple atomic stores. The type of the pointee must be an integer, pointer, or
-floating-point type whose bit width is a power of two greater than or equal to
+multiple atomic stores. The type of the pointee must be an integer, pointer,
+floating-point, or vector type whose bit width is a power of two greater than or equal to
 eight and less than or equal to a target-specific size limit.  ``align`` must be
 explicitly specified on atomic loads. Note: if the alignment is not greater or
 equal to the size of the `<value>` type, the atomic operation is likely to
@@ -11097,8 +11097,8 @@ If the ``store`` is marked as ``atomic``, it takes an extra :ref:`ordering
 <ordering>` and optional ``syncscope("<target-scope>")`` argument. The
 ``acquire`` and ``acq_rel`` orderings aren't valid on ``store`` instructions.
 Atomic loads produce :ref:`defined <memmodel>` results when they may see
-multiple atomic stores. The type of the pointee must be an integer, pointer, or
-floating-point type whose bit width is a power of two greater than or equal to
+multiple atomic stores. The type of the pointee must be an integer, pointer,
+floating-point, or vector type whose bit width is a power of two greater than or equal to
 eight and less than or equal to a target-specific size limit.  ``align`` must be
 explicitly specified on atomic stores. Note: if the alignment is not greater or
 equal to the size of the `<value>` type, the atomic operation is likely to
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 48e27763017be9..6f3d84c50ca206 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4259,9 +4259,10 @@ 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->getScalarType()->isIntOrPtrTy() ||
+              ElTy->getScalarType()->isFloatingPointTy(),
+          "atomic load operand must have integer, pointer, floating point, "
+          "or vector type!",
           ElTy, &LI);
     checkAtomicMemAccessSize(ElTy, &LI);
   } else {
@@ -4285,9 +4286,10 @@ 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->getScalarType()->isIntOrPtrTy() ||
+              ElTy->getScalarType()->isFloatingPointTy(),
+          "atomic store operand must have integer, pointer, floating point, "
+          "or vector type!",
           ElTy, &SI);
     checkAtomicMemAccessSize(ElTy, &SI);
   } else {
diff --git a/llvm/test/Assembler/atomic.ll b/llvm/test/Assembler/atomic.ll
index a44dcccc16bef1..f1027d5d3fbde4 100644
--- a/llvm/test/Assembler/atomic.ll
+++ b/llvm/test/Assembler/atomic.ll
@@ -52,6 +52,15 @@ define void @f(ptr %x) {
   ; CHECK: atomicrmw volatile usub_sat ptr %x, i32 10 syncscope("agent") monotonic
   atomicrmw volatile usub_sat ptr %x, i32 10 syncscope("agent") monotonic
 
+  ; CHECK : load atomic <1 x i32>, ptr %x unordered, align 4
+  load atomic <1 x i32>, ptr %x unordered, align 4
+  ; CHECK : store atomic <1 x i32> splat (i32 3), ptr %x release, align 4
+  store atomic <1 x i32> <i32 3>, ptr %x release, align 4
+  ; CHECK : load atomic <2 x i32>, ptr %x unordered, align 4
+  load atomic <2 x i32>, ptr %x unordered, align 4
+  ; CHECK : store atomic <2 x i32> <i32 3, i32 4>, ptr %x release, align 4
+  store atomic <2 x i32> <i32 3, i32 4>, ptr %x release, align 4
+
   ; CHECK: fence syncscope("singlethread") release
   fence syncscope("singlethread") release
   ; CHECK: fence seq_cst
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
 }

[jyknight]

There's a crash in AtomicExpandPass with vectors, llc -mtriple arm-none-none crashes on:

define <1 x ptr> @atomic_vec1_ptr(ptr %x) #0 {
  %ret = load atomic <1 x ptr>, ptr %x acquire, align 4
  ret <1 x ptr> %ret
}

llc: llvm/lib/IR/Instructions.cpp:2974: static llvm::CastInst *llvm::CastInst::Create(Instruction::CastOps, llvm::Value *, llvm::Type *, const llvm::Twine &, llvm::InsertPosition): Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.

Call at llvm/lib/CodeGen/AtomicExpandPass.cpp:2064:19 errors; it apparently tries to create a cast from i32 to <1 x ptr> with an inttoptr, but that doesn't work.

A lot more test coverage than is currently present in this series is going to be needed.

[jofrn]

There's a crash in AtomicExpandPass with vectors, llc -mtriple arm-none-none crashes on:

define <1 x ptr> @atomic_vec1_ptr(ptr %x) #0 {
  %ret = load atomic <1 x ptr>, ptr %x acquire, align 4
  ret <1 x ptr> %ret
}

llc: llvm/lib/IR/Instructions.cpp:2974: static llvm::CastInst *llvm::CastInst::Create(Instruction::CastOps, llvm::Value *, llvm::Type *, const llvm::Twine &, llvm::InsertPosition): Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.

Call at llvm/lib/CodeGen/AtomicExpandPass.cpp:2064:19 errors; it apparently tries to create a cast from i32 to <1 x ptr> with an inttoptr, but that doesn't work.

A lot more test coverage than is currently present in this series is going to be needed.

I also tried %ret = load atomic <2 x ptr>, ptr %x acquire, align 16 with llc, and it failed with an %2 = inttoptr i128 %1 to <2 x ptr>.

It seems we either need to overload on the atomic return type or give inttoptr the ability to cast between these.

[nikic]

Seeing the rest of the patch series, I'm a bit confused on what the semantics atomic vector load/store are actually supposed to me. My initial assumption was that load atomic <2 x i32> is, in terms of semantics, equivalent to load atomic i64 with appropriate bitcasts (for simplicity, ignoring any poison-propagation concerns). But it looks like you actually want it to be equivalent to two load atomic i32 instructions that then get assembled into a vector. That is, you do not guarantee that all components of the vector will be loaded atomically, only that each individual component is loaded atomically.

Either way, this needs to be clearly spelled out in LangRef.

[arsenm]

My initial assumption was that load atomic <2 x i32> is, in terms of semantics, equivalent to load atomic i64 with appropriate bitcasts

That is what this should be