[SROA] Prevent load atomic vector from being generated by jofrn · Pull Request #112432 · llvm/llvm-project

jofrn · 2024-10-15T20:15:10Z

load atomic <n x T> is illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

llvmbot · 2024-10-15T20:15:48Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: None (jofrn)

Changes

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/SROA.cpp (+5)
(added) llvm/test/Transforms/SROA/atomic-vector.ll (+19)

diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 92589ab17da313..450ecdf20ef009 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -2853,6 +2853,11 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {
 
   bool visitLoadInst(LoadInst &LI) {
     LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal
+    if (LI.isAtomic() && NewAI.getAllocatedType()->isVectorTy())
+      return false;
+
     Value *OldOp = LI.getOperand(0);
     assert(OldOp == OldPtr);
 
diff --git a/llvm/test/Transforms/SROA/atomic-vector.ll b/llvm/test/Transforms/SROA/atomic-vector.ll
new file mode 100644
index 00000000000000..d43ae653fba1dd
--- /dev/null
+++ b/llvm/test/Transforms/SROA/atomic-vector.ll
@@ -0,0 +1,19 @@
+; RUN: opt < %s -passes='sroa' -S 2>&1 | FileCheck %s --check-prefix=ERR
+; RUN: opt < %s -passes='sroa' -S | FileCheck %s
+
+define float @atomic_vector() {
+; ERR-NOT: atomic load operand must have integer, pointer, or floating point type!
+; ERR-NOT:   <1 x float>  {{%.*}} = load atomic volatile <1 x float>, ptr {{%.*}} acquire, align 4
+; CHECK:      %1 = alloca <1 x float>, align 4
+; CHECK-NEXT: store <1 x float> undef, ptr %1, align 4
+; CHECK-NEXT: %2 = load atomic volatile float, ptr %1 acquire, align 4
+; CHECK-NEXT: ret float %2
+  %1 = alloca <1 x float>
+  %2 = alloca <1 x float>
+  %3 = alloca ptr
+  call void @llvm.memcpy.p0.p0.i64(ptr %2, ptr %1, i64 4, i1 false)
+  store ptr %2, ptr %3
+  %4 = load ptr, ptr %3
+  %5 = load atomic volatile float, ptr %4 acquire, align 4
+  ret float %5
+}

jofrn · 2024-10-15T20:17:14Z

This is a follow-up to #111414, which used -disable-verify. This commit prevents atomic load vector at all.

shiltian · 2024-10-15T20:23:35Z

    LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal
+    if (LI.isAtomic() && NewAI.getAllocatedType()->isVectorTy())


Is it illegal from LangRef's perspective?

It is at least illegal from the perspective of IR/Verifier.

It looks like this is written here.

The restriction is dumb and we should relax it. Instead of just hardcoding isVectorTy here, should have some kind of LoadInst::isValidAtomicType or something

The verifier check should be moved into a LoadInst helper

No alloca reference, just the type. The implementation should match Verifier::visitLoadInst, the isIntOrIntPtrTy || isFloatingPointTy and checkAtomicMemAccessSize (also add some non-atomic size tests?)

We need to ensure an atomic does not have a vector type. If the load is atomic and the alloca that will lend its type over has a vector type, then we will generate an atomic vector, which are illegal. We need to ensure that this doesn't occur.

No, you need to ensure the atomic is a valid type for an atomic load.

Alternatively you can do the load with the equivalent sized type and then bitcast (which is why this restriction is dumb in the first place, the lowering can always do the same)

We are ensuring it is a valid type in the case under question, by checking the alloca's type. The invalid load will not be generated if the alloca has a vector type as that type will overwrite the load's type.

We don't want any loads that are atomic to have a vector type, regardless of whether the atomic itself is already valid; so checking if the atomic is more valid before translating it in AllocaSliceRewriter will miss cases where we form an invalid atomic during SROA (and then later form a vector type with it in visitLoadInst). Even though it is not likely as SROA probably won't form these, it illustrates why we don't need the extra checks here.

arsenm · 2024-10-16T04:11:57Z

+; CHECK-NEXT:    [[TMP2:%.*]] = load atomic volatile float, ptr [[TMP1]] acquire, align 4
+; CHECK-NEXT:    ret float [[TMP2]]
+;
+  %1 = alloca <1 x float>


Use named values in tests

arsenm · 2024-10-16T04:12:05Z

+; CHECK-NEXT:    ret float [[TMP2]]
+;
+  %1 = alloca <1 x float>
+  %2 = alloca <1 x float>


Test an integer and pointer vector too

jofrn · 2024-10-18T06:41:13Z

-; CHECK-NEXT:    ret i32 [[A_0_LOAD_EXT]]
+; CHECK-NEXT:    [[A:%.*]] = alloca i1, align 1
+; CHECK-NEXT:    [[V:%.*]] = load atomic i32, ptr [[A]] seq_cst, align 4
+; CHECK-NEXT:    ret i32 [[V]]


Due to this test, we may want to check only the type without DL. Adding too many checks hinders optimization.

jofrn · 2024-10-18T06:43:14Z

+  if (AO == AtomicOrdering::Release || AO == AtomicOrdering::AcquireRelease)
+    return false;
+  unsigned Size = DL.getTypeSizeInBits(Ty);
+  return Size >= 8 && !(Size & (Size - 1));


basictest.ll fails due to checking DL here.

arsenm · 2024-10-20T14:47:04Z

+  if (!Ty->isIntOrPtrTy() && !Ty->isFloatingPointTy())
+    return false;
+  return true;


Direct return of boolean expression

arsenm · 2024-10-20T14:47:29Z

  bool visitLoadInst(LoadInst &LI) {
    LLVM_DEBUG(dbgs() << "    original: " << LI << "\n");
+
+    // load atomic vector would be generated, which is illegal


Suggested change

// load atomic vector would be generated, which is illegal

// Load atomic vector would be generated, which is illegal.

arsenm · 2024-10-20T14:47:56Z

+  %indirect = load ptr, ptr %direct
+  %ret = load atomic volatile ptr, ptr %indirect acquire, align 4
+  ret ptr %ret
+}


Test a <2 x i16> or some other real vector. 1 x is a degenerate case

arsenm · 2024-10-20T22:28:27Z

  /// Returns false if this type would be invalid in the
  /// creation of a load atomic instruction.
-  static bool isValidAtomicTy(Type *Ty);
+  static bool isValidAtomicTy(Type *Ty, const DataLayout *DL = nullptr,


DataLayout must be mandatory

Then we need to omit it entirely because otherwise we will be checking the type size of the alloca, which we want to avoid if to optimize basictest.ll.

arsenm · 2024-10-20T22:28:31Z

    return false;
+  if (AO == AtomicOrdering::Release || AO == AtomicOrdering::AcquireRelease)
+    return false;
+  if (DL) {


arsenm · 2024-10-20T22:30:25Z

+  %indirect = load ptr, ptr %direct
+  %ret = load atomic volatile i32, ptr %indirect acquire, align 4
+  ret i32 %ret
+}


Add test for the non-byte illegal case?

RKSimon · 2026-07-06T16:04:04Z

@jofrn out of date patch?

dtcxzyw · 2026-07-06T16:47:26Z

The following correctness issue was found by llvm-hackme.

This comment is generated by an automated correctness checking service designed to help identify critical correctness bugs (opt crashes or Alive2 miscompilations) and improve PR review efficiency under limited reviewer bandwidth.

The reproducer was generated by LLM.

This bug is a regression introduced by this PR. It does not reproduce on the baseline commit.

Reproducer

Kind: crash

IR Reproducer:

; RUN: opt -passes=sroa -S
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define i32 @crash() {
  %a = alloca <2 x i32>, align 8
  store <2 x i32> <i32 1, i32 2>, ptr %a, align 8
  %v = load atomic i32, ptr %a monotonic, align 4
  ret i32 %v
}

Stacktrace:

opt: ../llvm-project-pr/llvm/lib/Transforms/Scalar/SROA.cpp:2812: bool {anonymous}::AllocaSliceRewriter::visit({anonymous}::AllocaSlices::const_iterator): Assertion `CanSROA' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug.
Stack dump:
0.	Program arguments: /llvm-hackme/work/llvm-hackme/llvm-build-pr/bin/opt -S -o /dev/null /tmp/tmpzapd5_sh.ll -passes=sroa
1.	Running pass "function(sroa<modify-cfg>)" on module "/tmp/tmpzapd5_sh.ll"
2.	Running pass "sroa<modify-cfg>" on function "crash"
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  libLLVMSupport.so.23.0git       0x00007ffff7e177f2 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 66
1  libLLVMSupport.so.23.0git       0x00007ffff7e1449c llvm::sys::RunSignalHandlers() + 76
2  libLLVMSupport.so.23.0git       0x00007ffff7e146a1
3  libc.so.6                       0x00007ffff7a19520
4  libc.so.6                       0x00007ffff7a6d9fc pthread_kill + 300
5  libc.so.6                       0x00007ffff7a19476 raise + 22
6  libc.so.6                       0x00007ffff79ff7f3 abort + 211
7  libc.so.6                       0x00007ffff79ff71b
8  libc.so.6                       0x00007ffff7a10e96
9  libLLVMScalarOpts.so.23.0git    0x00007fffef950b4b
10 libLLVMScalarOpts.so.23.0git    0x00007fffef953697
11 libLLVMScalarOpts.so.23.0git    0x00007fffef955e81
12 libLLVMScalarOpts.so.23.0git    0x00007fffef9577c7
13 libLLVMScalarOpts.so.23.0git    0x00007fffef958675 llvm::SROAPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) + 565
14 libLLVMPasses.so.23.0git        0x00007ffff14fe9c6
15 libLLVMCore.so.23.0git          0x00007fffedf00f4d llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) + 429
16 libLLVMAMDGPUCodeGen.so.23.0git 0x00007ffff5aa9586
17 libLLVMCore.so.23.0git          0x00007fffedeff9f1 llvm::ModuleToFunctionPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) + 545
18 libLLVMOptDriver.so.23.0git     0x00007ffff7f84d96
19 libLLVMCore.so.23.0git          0x00007fffedf00017 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) + 359
20 libLLVMOptDriver.so.23.0git     0x00007ffff7f91a5b llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool, bool) + 4923
21 libLLVMOptDriver.so.23.0git     0x00007ffff7f9ee84 optMain + 12452
22 libc.so.6                       0x00007ffff7a00d90
23 libc.so.6                       0x00007ffff7a00e40 __libc_start_main + 128
24 opt                             0x0000555555555095 _start + 37

Baseline Revision: 60cccae52fda78a9cdc9323900e14952d71b2624
PR Head SHA: e2e58ee18731db8cd3e10b3be69c6e88be23624b
Patch SHA256: 100c910b142564ec33f48dbe29fb406167f481deb2f2104b3e3866bdff20ff95

dtcxzyw

A correctness issue was identified by llvm-hackme. Please see the issue comment for details.

[SROA] Prevent load atomic vector from being generated

cb05b56

These are illegal, and they can be formed from SROA via indirect volatile loads in the AllocaSliceRewriter.

jofrn requested review from Pierre-vh, arsenm and shiltian October 15, 2024 20:15

llvmbot added the llvm:transforms label Oct 15, 2024

shiltian reviewed Oct 15, 2024

View reviewed changes

Comment thread llvm/test/Transforms/SROA/atomic-vector.ll Outdated

Autogenerate assertions

0c61505

jofrn force-pushed the atomicvec-sroa branch from 77b4341 to 0c61505 Compare October 15, 2024 21:18

arsenm reviewed Oct 16, 2024

View reviewed changes

Add i32,ptr tests and rename variables

083346d

jofrn force-pushed the atomicvec-sroa branch from 341ed5a to 4685582 Compare October 17, 2024 04:10

memcpy size must be >=8 to generate load atomic <1 x ptr>

cf47505

jofrn force-pushed the atomicvec-sroa branch from 4685582 to cf47505 Compare October 17, 2024 08:10

Use isValidAtomicTy

ef4fe92

llvmbot added the llvm:ir label Oct 18, 2024

jofrn added 2 commits October 17, 2024 23:48

Add AtomicOrdering to isValidAtomicTy interface

bdebaa8

Updated basictest.ll for checking getTypeSizeInBits

d68d923

jofrn commented Oct 18, 2024

View reviewed changes

Revert basictest.ll changes and DL check

1c1aae7

jofrn requested review from arsenm and shiltian October 18, 2024 16:44

arsenm reviewed Oct 20, 2024

View reviewed changes

Add <2 x i32> test. Add DL as optional to isValidAtomicTy

67e0560

arsenm reviewed Oct 20, 2024

View reviewed changes

jofrn force-pushed the atomicvec-sroa branch 2 times, most recently from 198a549 to db0a8fe Compare October 21, 2024 11:01

Added nonbyte test. Removed interface to isValidAtomicTy.

e2e58ee

jofrn force-pushed the atomicvec-sroa branch from db0a8fe to e2e58ee Compare October 21, 2024 11:04

jofrn requested a review from arsenm October 22, 2024 18:01

dtcxzyw requested changes Jul 6, 2026

View reviewed changes

	// load atomic vector would be generated, which is illegal
	// Load atomic vector would be generated, which is illegal.

Uh oh!

Conversation

jofrn commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jofrn commented Oct 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jofrn Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jofrn Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jofrn Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jofrn Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon commented Jul 6, 2026

Uh oh!

dtcxzyw commented Jul 6, 2026

Reproducer

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jofrn commented Oct 15, 2024 •

edited

Loading

llvmbot commented Oct 15, 2024 •

edited

Loading

jofrn Oct 15, 2024 •

edited

Loading

jofrn Oct 16, 2024 •

edited

Loading

jofrn Oct 16, 2024 •

edited

Loading

jofrn Oct 17, 2024 •

edited

Loading