[MLIR][LLVM] Fix non-deterministic alloca order in SROA under LLVM_REVERSE_ITERATION#192087
Conversation
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Mehdi Amini (joker-eph) ChangesAllocaOp::destructure iterated over usedIndices (SmallPtrSet) whose order depends on pointer values, causing allocas for destructured subslots to be emitted in a non-deterministic order when LLVM_REVERSE_ITERATION is enabled. Sort indices ascending by integer value before creating allocas to guarantee a stable output order. Update four test cases in sroa-intrinsics.mlir whose CHECK patterns relied on the old non-deterministic ordering. Assisted-by: Claude Code Full diff: https://github.com/llvm/llvm-project/pull/192087.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp
index 8f3c0dac026a7..1df14808bf076 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp
@@ -87,7 +87,12 @@ DenseMap<Attribute, MemorySlot> LLVM::AllocaOp::destructure(
auto destructurableType = cast<DestructurableTypeInterface>(getElemType());
DenseMap<Attribute, MemorySlot> slotMap;
- for (Attribute index : usedIndices) {
+ // Sort indices for deterministic alloca creation order.
+ SmallVector<Attribute> sortedIndices(usedIndices.begin(), usedIndices.end());
+ llvm::sort(sortedIndices, [](Attribute a, Attribute b) {
+ return cast<IntegerAttr>(a).getInt() < cast<IntegerAttr>(b).getInt();
+ });
+ for (Attribute index : sortedIndices) {
Type elemType = destructurableType.getTypeAtIndex(index);
assert(elemType && "used index must exist");
auto subAlloca = LLVM::AllocaOp::create(
diff --git a/mlir/test/Dialect/LLVMIR/sroa-intrinsics.mlir b/mlir/test/Dialect/LLVMIR/sroa-intrinsics.mlir
index 6dc8a97884ee1..3253077a76077 100644
--- a/mlir/test/Dialect/LLVMIR/sroa-intrinsics.mlir
+++ b/mlir/test/Dialect/LLVMIR/sroa-intrinsics.mlir
@@ -277,8 +277,8 @@ llvm.func @invalid_indirect_memset_inline() -> i32 {
// CHECK-LABEL: llvm.func @memset_double_use
llvm.func @memset_double_use() -> i32 {
// CHECK: %[[ALLOCA_LEN:.*]] = llvm.mlir.constant(1 : i32) : i32
- // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[ALLOCA_INT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x i32
+ // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[MEMSET_VALUE:.*]] = llvm.mlir.constant(42 : i8) : i8
%0 = llvm.mlir.constant(1 : i32) : i32
%1 = llvm.alloca %0 x !llvm.struct<"foo", (i32, f32)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
@@ -309,8 +309,8 @@ llvm.func @memset_double_use() -> i32 {
// CHECK-LABEL: llvm.func @memset_inline_double_use
llvm.func @memset_inline_double_use() -> i32 {
// CHECK: %[[ALLOCA_LEN:.*]] = llvm.mlir.constant(1 : i32) : i32
- // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[ALLOCA_INT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x i32
+ // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[MEMSET_VALUE:.*]] = llvm.mlir.constant(42 : i8) : i8
%0 = llvm.mlir.constant(1 : i32) : i32
%1 = llvm.alloca %0 x !llvm.struct<"foo", (i32, f32)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
@@ -398,8 +398,8 @@ llvm.func @memset_inline_considers_alignment() -> i32 {
// CHECK-LABEL: llvm.func @memset_considers_packing
llvm.func @memset_considers_packing() -> i32 {
// CHECK: %[[ALLOCA_LEN:.*]] = llvm.mlir.constant(1 : i32) : i32
- // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[ALLOCA_INT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x i32
+ // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[MEMSET_VALUE:.*]] = llvm.mlir.constant(42 : i8) : i8
%0 = llvm.mlir.constant(1 : i32) : i32
%1 = llvm.alloca %0 x !llvm.struct<"foo", packed (i8, i32, f32)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
@@ -430,8 +430,8 @@ llvm.func @memset_considers_packing() -> i32 {
// CHECK-LABEL: llvm.func @memset_inline_considers_packing
llvm.func @memset_inline_considers_packing() -> i32 {
// CHECK: %[[ALLOCA_LEN:.*]] = llvm.mlir.constant(1 : i32) : i32
- // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[ALLOCA_INT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x i32
+ // CHECK: %[[ALLOCA_FLOAT:.*]] = llvm.alloca %[[ALLOCA_LEN]] x f32
// CHECK: %[[MEMSET_VALUE:.*]] = llvm.mlir.constant(42 : i8) : i8
%0 = llvm.mlir.constant(1 : i32) : i32
%1 = llvm.alloca %0 x !llvm.struct<"foo", packed (i8, i32, f32)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
|
|
Coule these be sorted by their appearance order in the original type instead (for ease of reading of the output)? |
…VERSE_ITERATION AllocaOp::destructure iterated over usedIndices (SmallPtrSet) whose order depends on pointer values, causing allocas for destructured subslots to be emitted in a non-deterministic order when LLVM_REVERSE_ITERATION is enabled. Sort indices ascending by integer value before creating allocas to guarantee a stable output order. Update four test cases in sroa-intrinsics.mlir whose CHECK patterns relied on the old non-deterministic ordering. Assisted-by: Claude Code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
464ef16 to
c282685
Compare
I thought this is what was implemented? The sort was performed on their position, and the test changes show that now they appears in the order they were present in the original type. What am I missing? |
Moxinilian
left a comment
There was a problem hiding this comment.
Ah yes sorry I misread what the sorting was doing. I guess the new implementation is probably better anyway, so LGTM!
AllocaOp::destructure iterated over usedIndices (SmallPtrSet) whose order depends on pointer values, causing allocas for destructured subslots to be emitted in a non-deterministic order when LLVM_REVERSE_ITERATION is enabled. Sort indices ascending by integer value before creating allocas to guarantee a stable output order. Update four test cases in sroa-intrinsics.mlir whose CHECK patterns relied on the old non-deterministic ordering.
Assisted-by: Claude Code