[mlir][inline] Fix Issue#82401: Infinite loop in MLIR inliner for indirect recursive call. #124026
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 notified. 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 permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. 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. |
|
@llvm/pr-subscribers-mlir-core Author: None (junfengd-nv) ChangesIssue #82401 shows a problem where the MLIR inliner enters an infinite loop when call graph contains multiple indirect cycles. For example, the test case inlining-recursive-2.mlir contains cycles: parent1 -> child ->parent1 and parent2->child ->parent2. Current check in the shouldInline function fails to detect these cases. The fix is stop inlining when a pattern like A->B ->A is identified. Full diff: https://github.com/llvm/llvm-project/pull/124026.diff 3 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/Inliner.cpp b/mlir/lib/Transforms/Utils/Inliner.cpp
index 8acfc96d2b611b..756f5e379e7ddd 100644
--- a/mlir/lib/Transforms/Utils/Inliner.cpp
+++ b/mlir/lib/Transforms/Utils/Inliner.cpp
@@ -713,9 +713,11 @@ bool Inliner::Impl::shouldInline(ResolvedCall &resolvedCall) {
return false;
// Don't allow inlining if the target is a self-recursive function.
+ // Don't allow inlining if the call graph is like A->B->A.
if (llvm::count_if(*resolvedCall.targetNode,
[&](CallGraphNode::Edge const &edge) -> bool {
- return edge.getTarget() == resolvedCall.targetNode;
+ return edge.getTarget() == resolvedCall.targetNode ||
+ edge.getTarget() == resolvedCall.sourceNode;
}) > 0)
return false;
diff --git a/mlir/test/Transforms/inlining-recursive-2.mlir b/mlir/test/Transforms/inlining-recursive-2.mlir
new file mode 100644
index 00000000000000..7805c0ba348c2c
--- /dev/null
+++ b/mlir/test/Transforms/inlining-recursive-2.mlir
@@ -0,0 +1,37 @@
+// RUN: mlir-opt %s -inline='default-pipeline=''' | FileCheck %s
+// RUN: mlir-opt %s --mlir-disable-threading -inline='default-pipeline=''' | FileCheck %s
+
+module {
+ // CHECK-LABEL: func.func @parent1
+ func.func @parent1(%arg0: i32) -> i32 {
+ // CHECK: call @child
+ %0 = call @child(%arg0) : (i32) -> i32
+ return %0 : i32
+ }
+
+ // CHECK-LABEL: func.func @parent2
+ func.func @parent2(%arg0: i32) -> i32 {
+ // CHECK: call @child
+ %0 = call @child(%arg0) : (i32) -> i32
+ return %0 : i32
+ }
+
+ // CHECK-LABEL: func.func @child
+ func.func @child(%arg0: i32) -> i32 {
+ %c10_i32 = arith.constant 10 : i32
+ %c1_i32 = arith.constant 1 : i32
+ %0 = arith.cmpi sge, %arg0, %c10_i32 : i32
+ %1 = scf.if %0 -> (i32) {
+ scf.yield %arg0 : i32
+ } else {
+ %2 = arith.addi %arg0, %c1_i32 : i32
+ // CHECK: call @parent1
+ // CHECK: call @parent2
+ %3 = func.call @parent1(%2) : (i32) -> i32
+ %4 = func.call @parent2(%2) : (i32) -> i32
+ %5 = arith.addi %3, %4 : i32
+ scf.yield %5 : i32
+ }
+ return %1 : i32
+ }
+}
diff --git a/mlir/test/Transforms/inlining-recursive.mlir b/mlir/test/Transforms/inlining-recursive.mlir
index a02fe69133ad87..18b0f28a7cf37d 100644
--- a/mlir/test/Transforms/inlining-recursive.mlir
+++ b/mlir/test/Transforms/inlining-recursive.mlir
@@ -17,7 +17,7 @@ func.func @foo0(%arg0 : i32) -> i32 {
// CHECK-LABEL: func.func @foo1
func.func @foo1(%arg0 : i32) -> i32 {
- // CHECK: call @foo1
+ // CHECK: call @foo0
%0 = arith.constant 1 : i32
%1 = arith.subi %arg0, %0 : i32
%2 = call @foo0(%1) : (i32) -> i32
|
|
@llvm/pr-subscribers-mlir Author: None (junfengd-nv) ChangesIssue #82401 shows a problem where the MLIR inliner enters an infinite loop when call graph contains multiple indirect cycles. For example, the test case inlining-recursive-2.mlir contains cycles: parent1 -> child ->parent1 and parent2->child ->parent2. Current check in the shouldInline function fails to detect these cases. The fix is stop inlining when a pattern like A->B ->A is identified. Full diff: https://github.com/llvm/llvm-project/pull/124026.diff 3 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/Inliner.cpp b/mlir/lib/Transforms/Utils/Inliner.cpp
index 8acfc96d2b611b..756f5e379e7ddd 100644
--- a/mlir/lib/Transforms/Utils/Inliner.cpp
+++ b/mlir/lib/Transforms/Utils/Inliner.cpp
@@ -713,9 +713,11 @@ bool Inliner::Impl::shouldInline(ResolvedCall &resolvedCall) {
return false;
// Don't allow inlining if the target is a self-recursive function.
+ // Don't allow inlining if the call graph is like A->B->A.
if (llvm::count_if(*resolvedCall.targetNode,
[&](CallGraphNode::Edge const &edge) -> bool {
- return edge.getTarget() == resolvedCall.targetNode;
+ return edge.getTarget() == resolvedCall.targetNode ||
+ edge.getTarget() == resolvedCall.sourceNode;
}) > 0)
return false;
diff --git a/mlir/test/Transforms/inlining-recursive-2.mlir b/mlir/test/Transforms/inlining-recursive-2.mlir
new file mode 100644
index 00000000000000..7805c0ba348c2c
--- /dev/null
+++ b/mlir/test/Transforms/inlining-recursive-2.mlir
@@ -0,0 +1,37 @@
+// RUN: mlir-opt %s -inline='default-pipeline=''' | FileCheck %s
+// RUN: mlir-opt %s --mlir-disable-threading -inline='default-pipeline=''' | FileCheck %s
+
+module {
+ // CHECK-LABEL: func.func @parent1
+ func.func @parent1(%arg0: i32) -> i32 {
+ // CHECK: call @child
+ %0 = call @child(%arg0) : (i32) -> i32
+ return %0 : i32
+ }
+
+ // CHECK-LABEL: func.func @parent2
+ func.func @parent2(%arg0: i32) -> i32 {
+ // CHECK: call @child
+ %0 = call @child(%arg0) : (i32) -> i32
+ return %0 : i32
+ }
+
+ // CHECK-LABEL: func.func @child
+ func.func @child(%arg0: i32) -> i32 {
+ %c10_i32 = arith.constant 10 : i32
+ %c1_i32 = arith.constant 1 : i32
+ %0 = arith.cmpi sge, %arg0, %c10_i32 : i32
+ %1 = scf.if %0 -> (i32) {
+ scf.yield %arg0 : i32
+ } else {
+ %2 = arith.addi %arg0, %c1_i32 : i32
+ // CHECK: call @parent1
+ // CHECK: call @parent2
+ %3 = func.call @parent1(%2) : (i32) -> i32
+ %4 = func.call @parent2(%2) : (i32) -> i32
+ %5 = arith.addi %3, %4 : i32
+ scf.yield %5 : i32
+ }
+ return %1 : i32
+ }
+}
diff --git a/mlir/test/Transforms/inlining-recursive.mlir b/mlir/test/Transforms/inlining-recursive.mlir
index a02fe69133ad87..18b0f28a7cf37d 100644
--- a/mlir/test/Transforms/inlining-recursive.mlir
+++ b/mlir/test/Transforms/inlining-recursive.mlir
@@ -17,7 +17,7 @@ func.func @foo0(%arg0 : i32) -> i32 {
// CHECK-LABEL: func.func @foo1
func.func @foo1(%arg0 : i32) -> i32 {
- // CHECK: call @foo1
+ // CHECK: call @foo0
%0 = arith.constant 1 : i32
%1 = arith.subi %arg0, %0 : i32
%2 = call @foo0(%1) : (i32) -> i32
|
|
Thanks for the PR! This is definitely correct, but seems overly conservative. Do you know what the LLVM inliner does in this situation? LGTM for the PR for now though, as it does resolve the issue. Would be nice for the inliner to be more powerful though (if it can be). |
| @@ -0,0 +1,37 @@ | |||
| // RUN: mlir-opt %s -inline='default-pipeline=''' | FileCheck %s | |||
There was a problem hiding this comment.
Should remove the misleading, trailing '', which expands to an empty string in shells.
LLVM inliner does some inlining. parent1 inlined child. Then when LLVM inliner considers "child" function, parent1 is updated, it is recognized as a recursive call and won't be inlined into child. parent2 is inlined into child. when considering "parent2", "child" is recognized as a recursive call and won't be inlined. with -debug-only=inline. I can see LLVM inliner split SCC, that may be the reason it doesn't trap into infinite loop. " Inlined an internal call edge and split an SCC, retaining this to avoid infinite inlining." |
junfengd-nv
changed the title
|
Since @River707 approved, Junfeng will investigate improvements in follow up patches and does not have commit access, I'll merge on her behalf. Thanks for the reviews! |
|
@junfengd-nv 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! |
Issue #82401 shows a problem where the MLIR inliner enters an infinite loop when call graph contains multiple indirect cycles. For example, the test case inlining-recursive-2.mlir contains cycles: parent1 -> child ->parent1 and parent2->child ->parent2. Current check in the shouldInline function fails to detect these cases. The fix is stop inlining when a pattern like A->B ->A is identified.