[Mlir-commits] [mlir] 041b0a8 - [MLIR][Operation] Fix `isBeforeInBlock` crash bug mentioned in https://github.com/llvm/llvm-project/issues/60909 (#101172)

[llvmlistbot at llvm.org]

Author: wang-y-z
Date: 2024-09-21T23:40:52+08:00
New Revision: 041b0a81b05cb20f0ece49bc72d3f6f611095ee0

URL: https://github.com/llvm/llvm-project/commit/041b0a81b05cb20f0ece49bc72d3f6f611095ee0
DIFF: https://github.com/llvm/llvm-project/commit/041b0a81b05cb20f0ece49bc72d3f6f611095ee0.diff

LOG: [MLIR][Operation] Fix `isBeforeInBlock` crash bug mentioned in https://github.com/llvm/llvm-project/issues/60909 (#101172)

# summary
This MR fix `isBeforeInBlock` crash bug mentioned in
https://github.com/llvm/llvm-project/issues/60909. Fixes #60909.
# Trigger condition
1. A block only have one operation.
2. `block->isOpOrderValid()` is true, but `op->hasValidOrder()` is
false.
3. call: `op->isBeforeInBlock(op)`, compared with op itself.

Will crash on `assert(blockFront != blockBack && "expected more than one
operation");`

# Case study
Simplified repro case in
`mlir/test/Pass/scf2cf-print-liveness-crash.mlir`
When put `-convert-scf-to-cf -test-print-liveness` together in one cmd
line, the first pass will work normally and crash on the second pass.
Details please refer  https://github.com/llvm/llvm-project/issues/60909

# Solutions
option1. in `isBeforeInBlock`, check if block only have one operation
before step into `updateOrderIfNecessary`, if have only one, it must
return false
option2. in `isBeforeInBlock`, check if `this == other`, if true return
false
option3. fix `addNodeToList` logic

I prefer option3: 

When a block contains only one operation and the user calls
op->isBeforeInBlock(op), if block->isOpOrderValid() returns true,
updateOrderIfNecessary is called. If op->hasValidOrder() is false, it
will crash at the assertion assert(blockFront != blockBack && "expected
more than one operation");.

This behavior is abnormal and needs fixing. I discovered that after the
first pass of `-convert-scf-to-cf`, there is a block with only one
operation where the block order is valid but the operation order is
invalid, leading to a crash when `-test-print-liveness` pass runs.

---------

Co-authored-by: isaacw <isaacw at nvidia.com>

Added: 
    mlir/test/Pass/scf-to-cf-and-print-liveness.mlir

Modified: 
    mlir/lib/IR/Operation.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 8f28d61c732cdc..3272ece65ba531 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -406,7 +406,7 @@ void Operation::updateOrderIfNecessary() {
   assert(block && "expected valid parent");
 
   // If the order is valid for this operation there is nothing to do.
-  if (hasValidOrder())
+  if (hasValidOrder() || llvm::hasSingleElement(*block))
     return;
   Operation *blockFront = &block->front();
   Operation *blockBack = &block->back();

diff  --git a/mlir/test/Pass/scf-to-cf-and-print-liveness.mlir b/mlir/test/Pass/scf-to-cf-and-print-liveness.mlir
new file mode 100644
index 00000000000000..ae1d461667fe54
--- /dev/null
+++ b/mlir/test/Pass/scf-to-cf-and-print-liveness.mlir
@@ -0,0 +1,20 @@
+// RUN: mlir-opt %s -pass-pipeline="builtin.module(func.func(convert-scf-to-cf), func.func(test-print-liveness))"
+
+module {
+  func.func @for_if_for(%arg0: index, %arg1: index, %arg2: index, %arg3: i1) {
+    %cst = arith.constant dense<0.000000e+00> : tensor<128x32xf16>
+    %0 = scf.for %arg4 = %arg0 to %arg1 step %arg2 iter_args(%arg5 = %cst) -> (tensor<128x32xf16>) {
+      %1 = scf.if %arg3 -> (tensor<128x32xf16>) {
+        scf.yield %arg5 : tensor<128x32xf16>
+      } else {
+        %2 = scf.for %arg6 = %arg0 to %arg1 step %arg2 iter_args(%arg7 = %arg5) -> (tensor<128x32xf16>) {
+          scf.yield %arg7 : tensor<128x32xf16>
+        }
+        scf.yield %2 : tensor<128x32xf16>
+      }
+      scf.yield %1 : tensor<128x32xf16>
+    }
+    
+    return
+  }
+}
\ No newline at end of file