[mlir] Reland the dialect conversion hanging use fix #87297

rsuderman · 2024-04-02T00:46:36Z

Dialect conversion sometimes can have a hanging use of an argument. Ensured that argument uses are dropped before removing the block.

llvmbot · 2024-04-02T00:47:06Z

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir-linalg

Author: Rob Suderman (rsuderman)

Changes

Dialect conversion sometimes can have a hanging use of an argument. Ensured that argument uses are dropped before removing the block.

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

2 Files Affected:

(modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+2)
(modified) mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir (+13)

diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 8671c1008902a0..270ac0a0868960 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -279,6 +279,8 @@ class CreateBlockRewrite : public BlockRewrite {
     auto &blockOps = block->getOperations();
     while (!blockOps.empty())
       blockOps.remove(blockOps.begin());
+    for (auto arg : block->getArguments())
+      arg.dropAllUses();
     block->dropAllUses();
     if (block->getParent())
       block->erase();
diff --git a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
index 17eec593691860..6494e1b2719487 100644
--- a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
+++ b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
@@ -15,3 +15,16 @@ func.func @tensor_with_unknown_rank(%arg0: tensor<*xi8>) -> tensor<*xi8> {
   %0 = "tosa.abs"(%arg0) : (tensor<*xi8>) -> tensor<*xi8>
   return %0 : tensor<*xi8>
 }
+
+// -----
+
+// CHECK-LABEL: @unranked_add
+func.func @unranked_add(%arg0 : tensor<10x10xf32> , %arg1 : tensor<10x10xf32>, %arg2 : tensor<*xf32>) -> (tensor<10x10xf32>) {
+  // expected-error@+3 {{failed to legalize operation 'tosa.add'}}
+  %reduce = tosa.reduce_max %arg0 {axis = 1 : i32} : (tensor<10x10xf32>) -> tensor<10x1xf32>
+  %1 = tosa.add %reduce, %arg1 : (tensor<10x1xf32>, tensor<10x10xf32>) -> tensor<10x10xf32>
+  %0 = tosa.add %1, %arg2 : (tensor<10x10xf32>, tensor<*xf32>) -> tensor<*xf32>
+  %2 = tosa.reshape %0 {new_shape = array<i64: 10, 10>} : (tensor<*xf32>) -> tensor<10x10xf32>
+  return %2 : tensor<10x10xf32>
+}
+

llvmbot · 2024-04-02T00:47:06Z

@llvm/pr-subscribers-mlir

Author: Rob Suderman (rsuderman)

Changes

Dialect conversion sometimes can have a hanging use of an argument. Ensured that argument uses are dropped before removing the block.

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

2 Files Affected:

(modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+2)
(modified) mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir (+13)

diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 8671c1008902a0..270ac0a0868960 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -279,6 +279,8 @@ class CreateBlockRewrite : public BlockRewrite {
     auto &blockOps = block->getOperations();
     while (!blockOps.empty())
       blockOps.remove(blockOps.begin());
+    for (auto arg : block->getArguments())
+      arg.dropAllUses();
     block->dropAllUses();
     if (block->getParent())
       block->erase();
diff --git a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
index 17eec593691860..6494e1b2719487 100644
--- a/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
+++ b/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir
@@ -15,3 +15,16 @@ func.func @tensor_with_unknown_rank(%arg0: tensor<*xi8>) -> tensor<*xi8> {
   %0 = "tosa.abs"(%arg0) : (tensor<*xi8>) -> tensor<*xi8>
   return %0 : tensor<*xi8>
 }
+
+// -----
+
+// CHECK-LABEL: @unranked_add
+func.func @unranked_add(%arg0 : tensor<10x10xf32> , %arg1 : tensor<10x10xf32>, %arg2 : tensor<*xf32>) -> (tensor<10x10xf32>) {
+  // expected-error@+3 {{failed to legalize operation 'tosa.add'}}
+  %reduce = tosa.reduce_max %arg0 {axis = 1 : i32} : (tensor<10x10xf32>) -> tensor<10x1xf32>
+  %1 = tosa.add %reduce, %arg1 : (tensor<10x1xf32>, tensor<10x10xf32>) -> tensor<10x10xf32>
+  %0 = tosa.add %1, %arg2 : (tensor<10x10xf32>, tensor<*xf32>) -> tensor<*xf32>
+  %2 = tosa.reshape %0 {new_shape = array<i64: 10, 10>} : (tensor<*xf32>) -> tensor<10x10xf32>
+  return %2 : tensor<10x10xf32>
+}
+

matthias-springer · 2024-04-02T03:45:01Z

Looks like this is still leaking: https://lab.llvm.org/buildbot/#/builders/5/builds/42261

This reverts commit 49a4ec2.

stellaraccident · 2024-04-02T05:55:12Z

No good deed... :/

matthias-springer · 2024-04-02T06:58:19Z

I can take a look at this tomorrow. Making any change to the dialect conversion framework is a real pain if you're not super familiar with the code base.

This reverts commit 49a4ec2. Reason: Broke the ASan build bot with a memory leak. See the comments at #87297 for more information.

hctim · 2024-04-02T12:48:43Z

Looks like 2898178 (your revert) was in your own branch, not main. I've reverted it in main as well (56aeac4)

rsuderman · 2024-04-02T15:53:01Z

Looks like this is still leaking: https://lab.llvm.org/buildbot/#/builders/5/builds/42261

Lovely - yeah I tried to repro locally but I could not get ASAN to reproduce the issue. Something is dangling. I am betting there is a second change needed causing the leak that builds on top of the argument unlinking.

vitalybuka · 2024-04-02T16:00:24Z

Lovely - yeah I tried to repro locally but I could not get ASAN to reproduce the issue. Something is dangling. I am betting there is a second change needed causing the leak that builds on top of the argument unlinking.

Did you try https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild?
If fails on 3 out bots, 2 x86 and 1 arm, must be easily reproducible locally.

Operations must be created with the supplied builder. Otherwise, the dialect conversion / greedy pattern rewrite driver can break. This commit fixes a crash in the dialect conversion: ``` within split at llvm-project/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir:1 offset :8:8: error: failed to legalize operation 'tosa.add' %0 = tosa.add %1, %arg2 : (tensor<10x10xf32>, tensor<*xf32>) -> tensor<*xf32> ^ within split at llvm-project/mlir/test/Conversion/TosaToLinalg/tosa-to-linalg-invalid.mlir:1 offset :8:8: note: see current operation: %9 = "tosa.add"(%8, %arg2) : (tensor<10x10xf32>, tensor<*xf32>) -> tensor<*xf32> mlir-opt: llvm-project/mlir/include/mlir/IR/UseDefLists.h:198: mlir::IRObjectWithUseList<mlir::OpOperand>::~IRObjectWithUseList() [OperandType = mlir::OpOperand]: Assertion `use_empty() && "Cannot destroy a value that still has uses!"' failed. ``` This commit is the proper fix for #87297 (which was reverted).

[mlir] Reland the dialect conversion hanging use fix

fb0bee0

Dialect conversion sometimes can have a hanging use of an argument. Ensured that argument uses are dropped before removing the block.

rsuderman requested a review from matthias-springer April 2, 2024 00:46

llvmbot added mlir:core MLIR Core Infrastructure mlir:linalg mlir labels Apr 2, 2024

matthias-springer approved these changes Apr 2, 2024

View reviewed changes

rsuderman merged commit 49a4ec2 into llvm:main Apr 2, 2024

joker-eph added a commit that referenced this pull request Apr 2, 2024

Revert "[mlir] Reland the dialect conversion hanging use fix (#87297)"

2898178

This reverts commit 49a4ec2.

hctim added a commit that referenced this pull request Apr 2, 2024

Revert "[mlir] Reland the dialect conversion hanging use fix (#87297)"

56aeac4

This reverts commit 49a4ec2. Reason: Broke the ASan build bot with a memory leak. See the comments at #87297 for more information.

matthias-springer mentioned this pull request Apr 3, 2024

[mlir][linalg] Fix builder API usage in RegionBuilderHelper #87451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir] Reland the dialect conversion hanging use fix #87297

[mlir] Reland the dialect conversion hanging use fix #87297

Uh oh!

rsuderman commented Apr 2, 2024

Uh oh!

llvmbot commented Apr 2, 2024 •

edited

Loading

Uh oh!

llvmbot commented Apr 2, 2024

Uh oh!

matthias-springer commented Apr 2, 2024

Uh oh!

stellaraccident commented Apr 2, 2024

Uh oh!

matthias-springer commented Apr 2, 2024

Uh oh!

hctim commented Apr 2, 2024

Uh oh!

rsuderman commented Apr 2, 2024

Uh oh!

vitalybuka commented Apr 2, 2024

Uh oh!

Uh oh!

[mlir] Reland the dialect conversion hanging use fix #87297

[mlir] Reland the dialect conversion hanging use fix #87297

Uh oh!

Conversation

rsuderman commented Apr 2, 2024

Uh oh!

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

Uh oh!

Uh oh!

llvmbot commented Apr 2, 2024

Uh oh!

matthias-springer commented Apr 2, 2024

Uh oh!

stellaraccident commented Apr 2, 2024

Uh oh!

matthias-springer commented Apr 2, 2024

Uh oh!

hctim commented Apr 2, 2024

Uh oh!

rsuderman commented Apr 2, 2024

Uh oh!

vitalybuka commented Apr 2, 2024

Uh oh!

Uh oh!

llvmbot commented Apr 2, 2024 •

edited

Loading