[ConstraintElim] Use cond from header as upper bound on IV in exit BB. #94610

fhahn · 2024-06-06T12:55:35Z

For loops, we can use the condition in the loop header as upper bound on the compared induction in the unique exit block, if it exists. This can be done even if there are multiple in-loop edges to the unique exit block, as any other exit may only exit earlier.

More generally, we could add the OR of all exit conditions to the exit, but that's a possible future extension.

llvmbot · 2024-06-06T12:56:10Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

For loops, we can use the condition in the loop header as upper bound on the compared induction in the unique exit block, if it exists. This can be done even if there are multiple in-loop edges to the unique exit block, as any other exit may only exit earlier.

More generally, we could add the OR of all exit conditions to the exit, but that's a possible future extension.

Fixes #90417.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+17)
(modified) llvm/test/Transforms/ConstraintElimination/induction-condition-in-loop-exit.ll (+3-6)

diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 70bfa469193bf..4b3ef4d4c222c 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1031,6 +1031,23 @@ void State::addInfoForInductions(BasicBlock &BB) {
   WorkList.push_back(FactOrCheck::getConditionFact(
       DTN, CmpInst::ICMP_SLT, PN, B,
       ConditionTy(CmpInst::ICMP_SLE, StartValue, B)));
+
+  assert(!StepOffset.isNegative() && "induction must be increasing");
+  // Try to add condition from header to the unique exit block, if there is one.
+  // When exiting either with EQ or NE, we know that the induction value must be
+  // u<= B, as a different exit may exit earlier.
+  if (Pred == CmpInst::ICMP_EQ) {
+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(0);
+    if (L->getUniqueExitBlock() == EB)
+      WorkList.emplace_back(FactOrCheck::getConditionFact(
+          DT.getNode(EB), CmpInst::ICMP_ULE, A, B));
+  }
+  if (Pred == CmpInst::ICMP_NE) {
+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(1);
+    if (L->getUniqueExitBlock() == EB)
+      WorkList.emplace_back(FactOrCheck::getConditionFact(
+          DT.getNode(EB), CmpInst::ICMP_ULE, A, B));
+  }
 }
 
 void State::addInfoFor(BasicBlock &BB) {
diff --git a/llvm/test/Transforms/ConstraintElimination/induction-condition-in-loop-exit.ll b/llvm/test/Transforms/ConstraintElimination/induction-condition-in-loop-exit.ll
index 44ce82b51d707..86828b5e7f369 100644
--- a/llvm/test/Transforms/ConstraintElimination/induction-condition-in-loop-exit.ll
+++ b/llvm/test/Transforms/ConstraintElimination/induction-condition-in-loop-exit.ll
@@ -17,8 +17,7 @@ define i1 @multi_exiting_loop_eq_same_unique_exit_const_compare_known(ptr %s) {
 ; CHECK-NEXT:    [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1
 ; CHECK-NEXT:    br i1 [[LATCH_C]], label %[[LOOP_HEADER]], label %[[EXIT]]
 ; CHECK:       [[EXIT]]:
-; CHECK-NEXT:    [[T:%.*]] = icmp ult i32 [[IV]], 1235
-; CHECK-NEXT:    ret i1 [[T]]
+; CHECK-NEXT:    ret i1 true
 ;
 entry:
   br label %loop.header
@@ -175,8 +174,7 @@ define i1 @multi_exiting_loop_eq_same_unique_exit_var_compare_known(ptr %s, i32
 ; CHECK-NEXT:    [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1
 ; CHECK-NEXT:    br i1 [[LATCH_C]], label %[[LOOP_HEADER]], label %[[EXIT]]
 ; CHECK:       [[EXIT]]:
-; CHECK-NEXT:    [[T:%.*]] = icmp ule i32 [[IV]], [[N]]
-; CHECK-NEXT:    ret i1 [[T]]
+; CHECK-NEXT:    ret i1 true
 ;
 entry:
   br label %loop.header
@@ -214,8 +212,7 @@ define i1 @multi_exiting_loop_ne_same_unique_exit_const_compare_known(ptr %s) {
 ; CHECK-NEXT:    [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1
 ; CHECK-NEXT:    br i1 [[LATCH_C]], label %[[LOOP_HEADER]], label %[[EXIT]]
 ; CHECK:       [[EXIT]]:
-; CHECK-NEXT:    [[T:%.*]] = icmp ult i32 [[IV]], 1235
-; CHECK-NEXT:    ret i1 [[T]]
+; CHECK-NEXT:    ret i1 true
 ;
 entry:
   br label %loop.header

dtcxzyw · 2024-06-06T13:28:58Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+  // Try to add condition from header to the unique exit block, if there is one.
+  // When exiting either with EQ or NE, we know that the induction value must be
+  // u<= B, as a different exit may exit earlier.
+  if (Pred == CmpInst::ICMP_EQ) {


It is incorrect when the initial value of indvar is greater than the bound.
Alive2: https://alive2.llvm.org/ce/z/UYsbAQ

We need ConditionTy(CmpInst::ICMP_ULE, StartValue, B) here.

Great point thanks! Incorrectly assume that we only reach here for monotonically increasing cases, should have added tests... Done in b7b8d02

and update the code here

Extra tests for #94610.

PR Link: llvm/llvm-project#94610

dtcxzyw

LGTM.

dtcxzyw · 2024-06-06T15:27:12Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+  ConditionTy Precond;
+  if (!MonotonicallyIncreasingUnsigned)
+    Precond = {CmpInst::ICMP_ULE, StartValue, B};
+  if (Pred == CmpInst::ICMP_EQ) {
+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(0);
+    if (L->getUniqueExitBlock() == EB) {
+      WorkList.emplace_back(FactOrCheck::getConditionFact(
+          DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));
+    }
+  }
+  if (Pred == CmpInst::ICMP_NE) {
+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(1);
+    if (L->getUniqueExitBlock() == EB)
+      WorkList.emplace_back(FactOrCheck::getConditionFact(
+          DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));
+  }


Suggested change

ConditionTy Precond;

if (!MonotonicallyIncreasingUnsigned)

Precond = {CmpInst::ICMP_ULE, StartValue, B};

if (Pred == CmpInst::ICMP_EQ) {

BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(0);

if (L->getUniqueExitBlock() == EB) {

WorkList.emplace_back(FactOrCheck::getConditionFact(

DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));

}

}

if (Pred == CmpInst::ICMP_NE) {

BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(1);

if (L->getUniqueExitBlock() == EB)

WorkList.emplace_back(FactOrCheck::getConditionFact(

DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));

}

if (ICmpInst::isEquality(Pred)) {

ConditionTy Precond;

if (!MonotonicallyIncreasingUnsigned)

Precond = {CmpInst::ICMP_ULE, StartValue, B};

BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(Pred == CmpInst::ICMP_NE);

if (L->getUniqueExitBlock() == EB)

WorkList.emplace_back(FactOrCheck::getConditionFact(

DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));

}

It should be simpler :)

nikic · 2024-06-07T08:37:45Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(0);
+    if (L->getUniqueExitBlock() == EB) {
+      WorkList.emplace_back(FactOrCheck::getConditionFact(
+          DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));


Suggested change

DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));

DT.getNode(EB), CmpInst::ICMP_ULE, PN, B, Precond));

I found the switch from PN in all prior code to A here confusing.

nikic · 2024-06-07T08:42:06Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+  // u<= B, as a different exit may exit earlier.
+  ConditionTy Precond;
+  if (!MonotonicallyIncreasingUnsigned)
+    Precond = {CmpInst::ICMP_ULE, StartValue, B};


I'm confused, why do we only need this pre-condition if !MonotonicallyIncreasingUnsigned? Let's say we have a monotonic nuw addrec, a header eq exit where the start value is greater that is not taken and some other exit that is taken. Wouldn't we miscompile that?

Doesn't MonotonicallyIncreasingUnsigned imply that StartValue u<= ExitValue == B?

I don't think so. It just implies that PN is monotonically increasing from StartValue, but it doesn't make a statement about how that relates to B.

If I take your first test case from https://alive2.llvm.org/ce/z/UYsbAQ, the current patch still folds it to true.

Thanks, updated to always add the precond, originally also thought that MonotonicallyIncreasingUnsigned was implying StartValue <= B. Test added in 3d11b3d

nikic · 2024-06-07T08:45:34Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+  }
+  if (Pred == CmpInst::ICMP_NE) {
+    BasicBlock *EB = cast<BranchInst>(BB.getTerminator())->getSuccessor(1);
+    if (L->getUniqueExitBlock() == EB)


I'm not sure I understand the significance of the unique exit block here. Even if the exit is non-unique, wouldn't we be still add this condition fact to the header exit?

I'm not sure I understand the significance of the unique exit block here. Even if the exit is non-unique, wouldn't we be still add this condition fact to the header exit?

It may cause a miscompilation if there is a path from another exit block to the header exit.

Why? Isn't the argument here that even if we go through another exit, it can only exit earlier than the header, so the condition still holds? Why would it matter whether the other exit has a direct edge to the header exit vs going there indirectly?

It's not needed I think, removed the restriction (for the case where each exit block only has a single predecessor in the loop, the patch isn't needed). Tests added in 798754f

v01dXYZ · 2024-06-07T14:04:27Z

It seems to me only the loop with a predicate EQ/NE are supported. But the canonicalisation of the predicate for an AddRec happens in IndVarSimplify (linearFunctionTestReplace):

llvm-project/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp

Line 951 in 3b16630

linearFunctionTestReplace(Loop *L, BasicBlock *ExitingBB,
I really don't want to sound like a smart*ss but I tested with clang and I didn't manage to get the snippet from the issue to be optimised (even by replacing the predicate from <= to !=). I could have made a mistake though.

Additional test coverage for a miscompile in earlier versions of #94610.

Additional test coverage with multi-exit loops for #94610.

For loops, we can use the condition in the loop header as upper bound on the compared induction in the unique exit block, if it exists. This can be done even if there are multiple in-loop edges to the unique exit block, as any other exit may only exit earlier. More generally, we could add the OR of all exit conditions to the exit, but that's a possible future extension. Fixes llvm#90417.

nikic

LGTM if no compile-time impact.

Please double-check whether this actually fixes #90417 (with current phase ordering) and drop the line from the PR description if not.

Additional test coverage for a miscompile in earlier versions of llvm#94610.

Additional test coverage with multi-exit loops for llvm#94610.

fhahn · 2024-07-09T18:22:19Z

LGTM if no compile-time impact.

Please double-check whether this actually fixes #90417 (with current phase ordering) and drop the line from the PR description if not.

Thanks, compile-time impact looks neutral. As @v01dXYZ pointed out, it the current version doesn't apply to #90417 with the current phase ordering. I removed the Fixes... and I'll add some phase ordering tests. We would also need to handle phis with multiple incoming values (a separate improvement which would probably be good to have anyways)

llvm#94610) For loops, we can use the condition in the loop header as upper bound on the compared induction in the unique exit block, if it exists. This can be done even if there are multiple in-loop edges to the unique exit block, as any other exit may only exit earlier. More generally, we could add the OR of all exit conditions to the exit, but that's a possible future extension. PR: llvm#94610

fhahn requested review from nikic, dtcxzyw, XChy and efriedma-quic June 6, 2024 12:55

llvmbot added the llvm:transforms label Jun 6, 2024

fhahn mentioned this pull request Jun 6, 2024

[SimplifyIndVar] Push more users to worklist for simplifyUsers #93598

Open

dtcxzyw requested changes Jun 6, 2024

View reviewed changes

fhahn added a commit that referenced this pull request Jun 6, 2024

[ConstraintElim] Add induction tests with different start values.

b7b8d02

Extra tests for #94610.

fhahn force-pushed the ce-loop-header-exit branch from dc6c9f5 to ff5519f Compare June 6, 2024 14:39

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jun 6, 2024

pre-commit: test PR94610

5a5e5d1

PR Link: llvm/llvm-project#94610

dtcxzyw mentioned this pull request Jun 6, 2024

pre-commit: test PR94610 dtcxzyw/llvm-opt-benchmark#665

Closed

dtcxzyw approved these changes Jun 6, 2024

View reviewed changes

nikic reviewed Jun 7, 2024

View reviewed changes

fhahn added a commit that referenced this pull request Jun 28, 2024

[ConstraintElim] Add test for mis-compile due to #94610.

3d11b3d

Additional test coverage for a miscompile in earlier versions of #94610.

fhahn added a commit that referenced this pull request Jun 28, 2024

[ConstraintElim] Add multi-exit tests for #94610.

798754f

Additional test coverage with multi-exit loops for #94610.

fhahn added 3 commits June 29, 2024 09:28

!fixup properly handle non-monotonically increasing case.

474acb5

!fixup always emit precondition, don't check unique predecessor

d51a036

fhahn force-pushed the ce-loop-header-exit branch from ff5519f to d51a036 Compare June 29, 2024 09:36

nikic approved these changes Jul 1, 2024

View reviewed changes

lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024

[ConstraintElim] Add test for mis-compile due to llvm#94610.

62ce490

Additional test coverage for a miscompile in earlier versions of llvm#94610.

lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024

[ConstraintElim] Add multi-exit tests for llvm#94610.

85bc803

Additional test coverage with multi-exit loops for llvm#94610.

Merge branch 'main' into ce-loop-header-exit

e1af9a5

fhahn merged commit 5b92713 into llvm:main Jul 9, 2024
7 checks passed

fhahn deleted the ce-loop-header-exit branch July 9, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ConstraintElim] Use cond from header as upper bound on IV in exit BB. #94610

[ConstraintElim] Use cond from header as upper bound on IV in exit BB. #94610

fhahn commented Jun 6, 2024 •

edited

Loading

llvmbot commented Jun 6, 2024

dtcxzyw Jun 6, 2024

fhahn Jun 6, 2024

dtcxzyw left a comment

dtcxzyw Jun 6, 2024

nikic Jun 7, 2024

nikic Jun 7, 2024

dtcxzyw Jun 7, 2024

nikic Jun 7, 2024

fhahn Jun 29, 2024

nikic Jun 7, 2024

dtcxzyw Jun 7, 2024

nikic Jun 7, 2024

fhahn Jun 29, 2024

v01dXYZ commented Jun 7, 2024 •

edited

Loading

nikic left a comment

fhahn commented Jul 9, 2024

	DT.getNode(EB), CmpInst::ICMP_ULE, A, B, Precond));
	DT.getNode(EB), CmpInst::ICMP_ULE, PN, B, Precond));

[ConstraintElim] Use cond from header as upper bound on IV in exit BB. #94610

[ConstraintElim] Use cond from header as upper bound on IV in exit BB. #94610

Conversation

fhahn commented Jun 6, 2024 • edited Loading

llvmbot commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtcxzyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v01dXYZ commented Jun 7, 2024 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

fhahn commented Jul 9, 2024

fhahn commented Jun 6, 2024 •

edited

Loading

v01dXYZ commented Jun 7, 2024 •

edited

Loading