[LoopFusion] Forget loop and block dispositions after latch merge #166233
Conversation
|
Can you provide a test case? |
|
@llvm/pr-subscribers-llvm-transforms Author: Alireza Torabian (1997alireza) ChangesMerging the latches of loops may affect the dispositions, so they should be forgotten after the merge. This patch will fix the crash #164082. Full diff: https://github.com/llvm/llvm-project/pull/166233.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 19eccb9e17020..a80071f3f041f 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1796,14 +1796,15 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- // Forget block dispositions as well, so that there are no dangling
- // pointers to erased/free'ed blocks.
- SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
mergeLatch(FC0, FC1);
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
+
// Merge the loops.
SmallVector<BasicBlock *, 8> Blocks(FC1.L->blocks());
for (BasicBlock *BB : Blocks) {
@@ -2092,14 +2093,15 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- // Forget block dispositions as well, so that there are no dangling
- // pointers to erased/free'ed blocks.
- SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
mergeLatch(FC0, FC1);
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
+
// Merge the loops.
SmallVector<BasicBlock *, 8> Blocks(FC1.L->blocks());
for (BasicBlock *BB : Blocks) {
|
1997alireza
force-pushed
the
fix-loop-disposition-crash
branch
from
23d18dd to
4d11955
Compare
Added a test case that crashes without this patch. |
| @@ -0,0 +1,62 @@ | |||
| ; RUN: opt -passes=loop-fusion -disable-output -stats < %s 2>&1 | FileCheck -check-prefix=STAT %s | |||
There was a problem hiding this comment.
Add ; REQUIRES: asserts
-stats requires asserts.
| ;; for (int col = 0; col < 100; ++col) | ||
| ;; if (col != row) | ||
| ;; Array[row][col] = row + col; | ||
|
|
There was a problem hiding this comment.
I would suggest you to add the reason why this test case is added, i.e., something like Loop fusion should not crash because of ....
Also, you can rename this test case to "pr164082.ll".
| for.body8: ; preds = %for.inc14, %for.cond6.preheader | ||
| %indvars.iv = phi i64 [ 0, %for.cond6.preheader ], [ %indvars.iv.next, %for.inc14 ] | ||
| %1 = trunc i64 %indvars.iv to i32 | ||
| %2 = trunc i64 %indvars.iv29 to i32 |
There was a problem hiding this comment.
Could you simplify the test IR? For example you may get rid of all trunc instructions.
There was a problem hiding this comment.
if the IR is generated by the compiler let's leave it as is?
1997alireza
force-pushed
the
fix-loop-disposition-crash
branch
2 times, most recently
from
4860acd to
4ee925e
Compare
| mergeLatch(FC0, FC1); | ||
|
|
||
| // Forget block dispositions as well, so that there are no dangling | ||
| // pointers to erased/free'ed blocks. |
There was a problem hiding this comment.
// pointers to erased/free'ed blocks.
->
// pointers to erased/free'ed blocks. It should be done after mergeLatch() because...
Same for your change below.
Merging the latches of loops may affect the dispositions, so they should be forgotten after the merge. This patch will fix the crash due to using loop dispositions after forgetting them.
1997alireza
force-pushed
the
fix-loop-disposition-crash
branch
from
4ee925e to
584e47f
Compare
4 participants
Merging the latches of loops may affect the dispositions, so they should be forgotten after the merge. This patch will fix the crash #164082.