[MLIR][Operation] Fix isBeforeInBlock crash bug mentioned in https://github.com/llvm/llvm-project/issues/60909
#101172
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
Hi @joker-eph , could you please help to review this PR, thanks in advance |
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: wang-y-z (wang-y-z) ChangessummaryThis MR fix Trigger condition
Will crash on Case studySimplified repro case in Solutionsoption1. in both OK for me. Other> Why the I think Let's think about this in IR: When Full diff: https://github.com/llvm/llvm-project/pull/101172.diff 2 Files Affected:
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index b51357198b1ca..235d6ac9873ab 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -391,6 +391,7 @@ bool Operation::isBeforeInBlock(Operation *other) {
// parent.
if (!block->isOpOrderValid()) {
block->recomputeOpOrder();
+ // } else if (!llvm::hasSingleElement(*block)) {
} else {
// Update the order either operation if necessary.
updateOrderIfNecessary();
@@ -502,10 +503,14 @@ Block *llvm::ilist_traits<::mlir::Operation>::getContainingBlock() {
/// keep the block pointer up to date.
void llvm::ilist_traits<::mlir::Operation>::addNodeToList(Operation *op) {
assert(!op->getBlock() && "already in an operation block!");
- op->block = getContainingBlock();
+
+ Block *curParent = getContainingBlock();
+ op->block = curParent;
// Invalidate the order on the operation.
op->orderIndex = Operation::kInvalidOrderIdx;
+ // Invalidate the ordering of the parent block.
+ curParent->invalidateOpOrder();
}
/// This is a trait method invoked when an operation is removed from a block.
diff --git a/mlir/test/Pass/scf2cf-print-liveness-crash.mlir b/mlir/test/Pass/scf2cf-print-liveness-crash.mlir
new file mode 100644
index 0000000000000..665e085cfedee
--- /dev/null
+++ b/mlir/test/Pass/scf2cf-print-liveness-crash.mlir
@@ -0,0 +1,19 @@
+// 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
|
|
@River707 can you double check this? Wanna make sure I didn't miss something, this seems too obvious of a bug. |
Let me double check, the addNodeToList method was supposed to be in-combination with another method (that would actually invalidate the list). |
|
Sorry, didn't have the time to look at this PR until now. I don't think the solution here is the one we want. It'll be overly conservative and thrash the order of a block every time an operation gets added (which makes the dominance checks a lot more expensive when transformations happen). Can we instead just update |
Thanks for pointing this from performance perspective, and I had a minor fix on |
There was a problem hiding this comment.
Thanks for fixing this. I've recently encountered this problem. :)
I am not familiar with this part of the compiler, but I think @River707 or @joker-eph are!
|
@wang-y-z Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
summary
This MR fix
isBeforeInBlockcrash bug mentioned in #60909. Fixes #60909.Trigger condition
block->isOpOrderValid()is true, butop->hasValidOrder()is false.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.mlirWhen put
-convert-scf-to-cf -test-print-livenesstogether in one cmd line, the first pass will work normally and crash on the second pass.Details please refer #60909
Solutions
option1. in
isBeforeInBlock, check if block only have one operation before step intoupdateOrderIfNecessary, if have only one, it must return falseoption2. in
isBeforeInBlock, check ifthis == other, if true return falseoption3. fix
addNodeToListlogicI 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-livenesspass runs.