From 884beda9dec50f549546976e5d343777ee01fee7 Mon Sep 17 00:00:00 2001 From: Simone Pellegrini Date: Thu, 5 Jun 2025 09:21:08 +0200 Subject: [PATCH] [mlir][vector] Fix attaching write effects on transfer_write's base This fixes an issue with `TransferWriteOp`'s implementation of the `MemoryEffectOpInterface` where the write effect was attached to the stored value rather than the base. This had the effect that when asking for the memory effects for the input memref buffer using `getEffectsOnValue(...)`, the function would return no-effects (as the effect would have been attached to the stored value rather than the input buffer). --- flang/test/HLFIR/assign-side-effects.fir | 9 +- flang/test/HLFIR/memory-effects.fir | 93 ++++++++++--------- mlir/lib/Dialect/Vector/IR/VectorOps.cpp | 2 +- .../Dialect/Bufferization/side-effects.mlir | 6 +- mlir/test/Dialect/Vector/side-effects.mlir | 15 +++ mlir/test/IR/test-side-effects.mlir | 8 +- mlir/test/lib/IR/TestSideEffects.cpp | 12 +-- 7 files changed, 81 insertions(+), 64 deletions(-) create mode 100644 mlir/test/Dialect/Vector/side-effects.mlir diff --git a/flang/test/HLFIR/assign-side-effects.fir b/flang/test/HLFIR/assign-side-effects.fir index dfd1c5886e4fa..cac9530e2277c 100644 --- a/flang/test/HLFIR/assign-side-effects.fir +++ b/flang/test/HLFIR/assign-side-effects.fir @@ -2,14 +2,14 @@ // RUN: fir-opt %s --test-side-effects --verify-diagnostics func.func @test1(%x: !fir.ref, %i: i32) { - // expected-remark @below {{found an instance of 'write' on a op operand, on resource ''}} + // expected-remark @below {{found an instance of 'write' on op operand 1, on resource ''}} hlfir.assign %i to %x : i32, !fir.ref return } func.func @test2(%x: !fir.ref, %y: !fir.ref) { - // expected-remark @below {{found an instance of 'write' on a op operand, on resource ''}} - // expected-remark @below {{found an instance of 'read' on a op operand, on resource ''}} + // expected-remark @below {{found an instance of 'write' on op operand 1, on resource ''}} + // expected-remark @below {{found an instance of 'read' on op operand 0, on resource ''}} hlfir.assign %y to %x : !fir.ref, !fir.ref return } @@ -22,7 +22,8 @@ func.func @test3(%x: !fir.ref>, %y: !fir.ref>) { } func.func @test4(%x: !fir.ref>>>, %y: !fir.box>) { - // expected-remark @below {{found an instance of 'read' on a op operand, on resource ''}} + // expected-remark @below {{found an instance of 'read' on op operand 0, on resource ''}} + // expected-remark @below {{found an instance of 'read' on op operand 1, on resource ''}} // expected-remark @below {{found an instance of 'write' on resource ''}} // expected-remark @below {{found an instance of 'free' on resource ''}} // expected-remark @below {{found an instance of 'allocate' on resource ''}} diff --git a/flang/test/HLFIR/memory-effects.fir b/flang/test/HLFIR/memory-effects.fir index cac887ebe67de..6c791f1260be7 100644 --- a/flang/test/HLFIR/memory-effects.fir +++ b/flang/test/HLFIR/memory-effects.fir @@ -3,8 +3,9 @@ func.func @concat(%arg0: !fir.ref>, %arg1: !fir.ref>) { // expected-remark@+1 {{operation has no memory effects}} %c30 = arith.constant 30 : index -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+3 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 1, on resource ''}} %0 = hlfir.concat %arg0, %arg1 len %c30 : (!fir.ref>, !fir.ref>, index) -> (!hlfir.expr>) return } @@ -16,8 +17,8 @@ func.func @all_no_effects(%arg0: !hlfir.expr<2x!fir.logical<4>>) { } func.func @all_effects(%arg0: !fir.ref>>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %all = hlfir.all %arg0 dim %arg1 : (!fir.ref>>, i32) -> !hlfir.expr> return } @@ -29,8 +30,8 @@ func.func @any_no_effects(%arg0: !hlfir.expr<2x!fir.logical<4>>) { } func.func @any_effects(%arg0: !fir.ref>>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %all = hlfir.any %arg0 dim %arg1 : (!fir.ref>>, i32) -> !hlfir.expr> return } @@ -42,7 +43,7 @@ func.func @count_no_effects(%arg0: !hlfir.expr<2x!fir.logical<4>>) { } func.func @count_effects(%arg0: !fir.ref>>, %arg1: i32) { -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %all = hlfir.count %arg0 dim %arg1 : (!fir.ref>>, i32) -> i32 return } @@ -54,15 +55,15 @@ func.func @product_no_effects(%arg0: !hlfir.expr) { } func.func @product_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %product = hlfir.product %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xf32> return } func.func @set_length_read(%arg0: !fir.ref>, %arg1: index) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %0 = hlfir.set_length %arg0 len %arg1 : (!fir.ref>, index) -> !hlfir.expr> return } @@ -74,8 +75,8 @@ func.func @sum_no_effects(%arg0: !hlfir.expr) { } func.func @sum_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %sum = hlfir.sum %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xf32> return } @@ -87,8 +88,8 @@ func.func @maxval_no_effects(%arg0: !hlfir.expr) { } func.func @maxval_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %maxval = hlfir.maxval %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xf32> return } @@ -100,34 +101,34 @@ func.func @minval_no_effects(%arg0: !hlfir.expr) { } func.func @minval_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %minval = hlfir.minval %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xf32> return } func.func @minloc_effects_simple(%arg0: !hlfir.expr) { -// expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} +// expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %minloc = hlfir.minloc %arg0 : (!hlfir.expr) -> !hlfir.expr return } func.func @minloc_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %minloc = hlfir.minloc %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xi32> return } func.func @maxloc_effects_simple(%arg0: !hlfir.expr) { -// expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} +// expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %maxloc = hlfir.maxloc %arg0 : (!hlfir.expr) -> !hlfir.expr return } func.func @maxloc_effects(%arg0: !fir.ref>, %arg1: i32) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %maxloc = hlfir.maxloc %arg0 dim %arg1 : (!fir.ref>, i32) -> !hlfir.expr<2xi32> return } @@ -139,49 +140,49 @@ func.func @dot_product_no_effects(%arg0: !hlfir.expr, %arg1: !hlfir.expr< } func.func @dot_product_effects(%arg0: !fir.ref>, %arg1: !fir.ref>) { -// there are read effects on both arguments - the diagnostic verification just doesn't register duplicate identical diagnostics -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 1, on resource ''}} %0 = hlfir.dot_product %arg0 %arg1 : (!fir.ref>, !fir.ref>) -> f32 return } func.func @matmul_no_reads(%arg0: !hlfir.expr, %arg1: !hlfir.expr) { -// expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} +// expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %0 = hlfir.matmul %arg0 %arg1 : (!hlfir.expr, !hlfir.expr) -> !hlfir.expr return } func.func @matmul_reads(%arg0: !fir.ref>, %arg1: !fir.ref>) { -// expected-remark@+3 {{found an instance of 'allocate' on a op result, on resource ''}} -// there are read effects on both arguments - the diagnostic verification just doesn't register duplicate identical diagnostics -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+3 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 1, on resource ''}} %0 = hlfir.matmul %arg0 %arg1 : (!fir.ref>, !fir.ref>) -> !hlfir.expr<10x10xf32> return } func.func @transpose_no_reads(%arg0: !hlfir.expr) { -// expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} +// expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %0 = hlfir.transpose %arg0 : (!hlfir.expr) -> !hlfir.expr return } func.func @transpose_read(%arg0: !fir.ref>) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %0 = hlfir.transpose %arg0 : (!fir.ref>) -> !hlfir.expr<5x10xf32> return } func.func @matmul_transpose_no_reads(%arg0: !hlfir.expr, %arg1: !hlfir.expr) { -// expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} +// expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %0 = hlfir.matmul_transpose %arg0 %arg1 : (!hlfir.expr, !hlfir.expr) -> !hlfir.expr return } func.func @matmul_transpose_reads(%arg0: !fir.ref>, %arg1: !fir.ref>) { -// expected-remark@+3 {{found an instance of 'allocate' on a op result, on resource ''}} -// there are read effects on both arguments - the diagnostic verification just doesn't register duplicate identical diagnostics -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+3 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 1, on resource ''}} %0 = hlfir.matmul_transpose %arg0 %arg1 : (!fir.ref>, !fir.ref>) -> !hlfir.expr<10x10xf32> return } @@ -195,8 +196,8 @@ func.func @associate(%arg0: i32) { } func.func @as_expr_read(%arg0: !fir.ref>) { -// expected-remark@+2 {{found an instance of 'allocate' on a op result, on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} %0 = hlfir.as_expr %arg0 : (!fir.ref>) -> !hlfir.expr // expected-remark@+1 {{found an instance of 'free' on resource ''}} hlfir.destroy %0 : !hlfir.expr @@ -204,28 +205,28 @@ func.func @as_expr_read(%arg0: !fir.ref>) { } func.func @char_extremum(%arg0: !fir.ref>, %arg1: !fir.ref>) { -// expected-remark@+3 {{found an instance of 'allocate' on a op result, on resource ''}} -// there are read effects on both arguments - the diagnostic verification just doesn't register duplicate identical diagnostics -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+3 {{found an instance of 'allocate' on op result 0, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 1, on resource ''}} %0 = hlfir.char_extremum min, %arg0, %arg1 : (!fir.ref>, !fir.ref>) -> !hlfir.expr> return } func.func @copy_in(%box: !fir.box>, %temp: !fir.ref>>>, %is_present: i1) { // expected-remark@+3 {{found an instance of 'allocate' on resource ''}} -// expected-remark@+2 {{found an instance of 'read' on a op operand, on resource ''}} -// expected-remark@+1 {{found an instance of 'write' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'write' on op operand 1, on resource ''}} %0:2 = hlfir.copy_in %box to %temp : (!fir.box>, !fir.ref>>>) -> (!fir.box>, i1) return } func.func @copy_out(%box: !fir.box>, %temp: !fir.ref>>>, %was_copied: i1) { // expected-remark@+2 {{found an instance of 'free' on resource ''}} -// expected-remark@+1 {{found an instance of 'read' on a op operand, on resource ''}} +// expected-remark@+1 {{found an instance of 'read' on op operand 0, on resource ''}} hlfir.copy_out %temp, %was_copied : (!fir.ref>>>, i1) -> () // expected-remark@+3 {{found an instance of 'free' on resource ''}} -// expected-remark@+2 {{found an instance of 'read' on a op operand, on resource ''}} -// expected-remark@+1 {{found an instance of 'write' on a op operand, on resource ''}} +// expected-remark@+2 {{found an instance of 'read' on op operand 0, on resource ''}} +// expected-remark@+1 {{found an instance of 'write' on op operand 2, on resource ''}} hlfir.copy_out %temp, %was_copied to %box : (!fir.ref>>>, i1, !fir.box>) -> () return } diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp index fcfb401fd9867..bd926b82cf14f 100644 --- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp +++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp @@ -5034,7 +5034,7 @@ void TransferWriteOp::getEffects( SmallVectorImpl> &effects) { if (llvm::isa(getShapedType())) - effects.emplace_back(MemoryEffects::Write::get(), &getValueToStoreMutable(), + effects.emplace_back(MemoryEffects::Write::get(), &getBaseMutable(), SideEffects::DefaultResource::get()); } diff --git a/mlir/test/Dialect/Bufferization/side-effects.mlir b/mlir/test/Dialect/Bufferization/side-effects.mlir index 841490e9f3234..129fc8b32c270 100644 --- a/mlir/test/Dialect/Bufferization/side-effects.mlir +++ b/mlir/test/Dialect/Bufferization/side-effects.mlir @@ -1,9 +1,9 @@ // RUN: mlir-opt %s --test-side-effects --verify-diagnostics func.func @test_side_effects(%arg0: memref<2xi32>) -> memref<2xi32> { - // expected-remark @below {{found an instance of 'read' on a op operand, on resource ''}} - // expected-remark @below {{found an instance of 'write' on a op result, on resource ''}} - // expected-remark @below {{found an instance of 'allocate' on a op result, on resource ''}} + // expected-remark @below {{found an instance of 'read' on op operand 0, on resource ''}} + // expected-remark @below {{found an instance of 'write' on op result 0, on resource ''}} + // expected-remark @below {{found an instance of 'allocate' on op result 0, on resource ''}} %0 = bufferization.clone %arg0 : memref<2xi32> to memref<2xi32> return %0 : memref<2xi32> } diff --git a/mlir/test/Dialect/Vector/side-effects.mlir b/mlir/test/Dialect/Vector/side-effects.mlir new file mode 100644 index 0000000000000..54c274a1a2a02 --- /dev/null +++ b/mlir/test/Dialect/Vector/side-effects.mlir @@ -0,0 +1,15 @@ +// RUN: mlir-opt %s --test-side-effects --verify-diagnostics + +func.func @test_side_effects(%arg0: memref<8xf32>) { + // expected-remark @below {{operation has no memory effects}} + %c0 = arith.constant 0 : index + // expected-remark @below {{operation has no memory effects}} + %c4 = arith.constant 4 : index + // expected-remark @below {{operation has no memory effects}} + %cst = arith.constant 0.0 : f32 + // expected-remark @below {{found an instance of 'read' on op operand 0, on resource ''}} + %0 = vector.transfer_read %arg0[%c0], %cst : memref<8xf32>, vector<4xf32> + // expected-remark @below {{found an instance of 'write' on op operand 1, on resource ''}} + vector.transfer_write %0, %arg0[%c4] : vector<4xf32>, memref<8xf32> + return +} diff --git a/mlir/test/IR/test-side-effects.mlir b/mlir/test/IR/test-side-effects.mlir index efce4856041a1..b652ecb7dad1d 100644 --- a/mlir/test/IR/test-side-effects.mlir +++ b/mlir/test/IR/test-side-effects.mlir @@ -15,7 +15,7 @@ func.func @side_effect(%arg : index) { {effect="write", test_resource} ]} : () -> i32 - // expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} + // expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %3 = "test.side_effect_op"() {effects = [ {effect="allocate", on_result, test_resource} ]} : () -> i32 @@ -38,19 +38,19 @@ func.func @side_effect(%arg : index) { effect_parameter = affine_map<(i, j) -> (j, i)> } : () -> i32 - // expected-remark@+1 {{found an instance of 'allocate' on a op operand, on resource ''}} + // expected-remark@+1 {{found an instance of 'allocate' on op operand 0, on resource ''}} %6 = test.side_effect_with_region_op (%arg) { ^bb0(%arg0 : index): test.region_yield %arg0 : index } {effects = [ {effect="allocate", on_operand, test_resource} ]} : index -> index - // expected-remark@+1 {{found an instance of 'allocate' on a op result, on resource ''}} + // expected-remark@+1 {{found an instance of 'allocate' on op result 0, on resource ''}} %7 = test.side_effect_with_region_op (%arg) { ^bb0(%arg0 : index): test.region_yield %arg0 : index } {effects = [ {effect="allocate", on_result, test_resource} ]} : index -> index - // expected-remark@+1 {{found an instance of 'allocate' on a block argument, on resource ''}} + // expected-remark@+1 {{found an instance of 'allocate' on block argument 0, on resource ''}} %8 = test.side_effect_with_region_op (%arg) { ^bb0(%arg0 : index): test.region_yield %arg0 : index diff --git a/mlir/test/lib/IR/TestSideEffects.cpp b/mlir/test/lib/IR/TestSideEffects.cpp index 7e01509d55685..000e7c204fd5f 100644 --- a/mlir/test/lib/IR/TestSideEffects.cpp +++ b/mlir/test/lib/IR/TestSideEffects.cpp @@ -52,12 +52,12 @@ struct SideEffectsPass diag << "'write'"; if (instance.getValue()) { - if (instance.getEffectValue()) - diag << " on a op operand,"; - else if (instance.getEffectValue()) - diag << " on a op result,"; - else if (instance.getEffectValue()) - diag << " on a block argument,"; + if (auto *opOpd = instance.getEffectValue()) + diag << " on op operand " << opOpd->getOperandNumber() << ","; + else if (auto opRes = instance.getEffectValue()) + diag << " on op result " << opRes.getResultNumber() << ","; + else if (auto opBlk = instance.getEffectValue()) + diag << " on block argument " << opBlk.getArgNumber() << ","; } else if (SymbolRefAttr symbolRef = instance.getSymbolRef()) diag << " on a symbol '" << symbolRef << "',";