<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Ok, mostly for history preservation sake...</p>
<p>As I'd expected yesterday, the reverted code was incorrect.
Essentially, the error I'd made is that when I ported the logic
from <span class="phui-oi-objname" data-sigil="ungrabbable">D109457
to the corresponding indvars reviews, I'd only considered one
aspect of the ControlsExit precondition from the SCEV code. I'd
gotten the bit about this being the sole exit, but I had not
included the requirement that a true condition led to the loop
exit. This hadn't been noticed in the original review, but once
I started generalizing the code in tree, it became pretty
obvious pretty quickly.</span></p>
<p><span class="phui-oi-objname" data-sigil="ungrabbable">In theory,
the fix for this would be fairly "obvious", but I'm not going to
make it. Instead, I'm just going to drop pushing this logic
entirely. Why? Because, a) there had been some resistance to
the notion of optimizing based on mustprgress during review, b)
it's turned out to be somewhat bugprone, c) I still think the
"right" place for this logic is SCEV, and d) I've been able to
cover all of the original motivating cases with the range based
logic. That last bit was a recent surprise, and causes me to
shift approach here. <br>
</span></p>
<p><span class="phui-oi-objname" data-sigil="ungrabbable">What we're
left with in tree is a purely range based mechanism to
canonicalize signed to unsigned conditions, and then narrow
(e.g. rotate) if legal. Since the original patches went out for
review, Roman also added CVP logic to do the same. SCEV and CVP
handle slightly non-overlapping cases (in particular, SCEV is
smarter about dominating conditions with more complicated
shapes), so between the two of them we get pretty decent
coverage on the signed to unsigned conversion, and some coverage
on the narrowing transformation. <br>
</span></p>
<p><span class="phui-oi-objname" data-sigil="ungrabbable">Given the
simplification of not worrying about the must-exit clauses, the
current in tree code is slightly more complicated than it needs
to be. I'm going to give the current code (with some extensions
landed today) a couple of days to bake, and then do a cleanup
pass to reduce complexity and update comments.</span></p>
<p><span class="phui-oi-objname" data-sigil="ungrabbable">Philip<br>
</span></p>
<div class="moz-cite-prefix">On 11/3/21 3:20 PM, Philip Reames via
llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:61830b13.1c69fb81.61496.87b7@mx.google.com">
<pre class="moz-quote-pre" wrap="">
Author: Philip Reames
Date: 2021-11-03T15:19:49-07:00
New Revision: d4708fa480f2894f3cc007cbcb8aaf2ad754d34b
URL: <a class="moz-txt-link-freetext" href="https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b">https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b</a>
DIFF: <a class="moz-txt-link-freetext" href="https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b.diff">https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b.diff</a>
LOG: Backout must-exit based parts of 3fc9882e, and 412eb0
Not sure these are correct. I think I missed a case when porting this from the original SCEV change to the IndVar changes. I may end up reapplying this later with a comment about how this is correct, but in case the current bad feeling turns out to be true, I'm removing from tree while investigating further.
Added:
Modified:
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
index fd8b08bed147..af7fbd092cbf 100644
--- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -1461,27 +1461,6 @@ bool IndVarSimplify::canonicalizeExitCondition(Loop *L) {
// have not changed exit counts, or the values produced by the compare.
continue;
}
-
- // If we have a loop which would be undefined if infinite, and it has at
- // most one possible dynamic exit, then we can conclude that exit must
- // be taken. If that exit must be taken, and we know the LHS can only
- // take values in the positive domain, then we can conclude RHS must
- // also be in that same range, and replace a signed compare with an
- // unsigned one.
- // If the exit might not be taken in a well defined program.
- if (ExitingBlocks.size() == 1 && SE->loopHasNoAbnormalExits(L) &&
- SE->loopIsFiniteByAssumption(L)) {
- // We have now matched icmp signed-cond zext(X), zext(Y'), and can thus
- // replace the signed condition with the unsigned version.
- ICmp->setPredicate(ICmp->getUnsignedPredicate());
- Changed = true;
-
- // Given we've changed exit counts, notify SCEV.
- // Some nested loops may share same folded exit basic block,
- // thus we need to notify top most loop.
- SE->forgetTopmostLoop(L);
- continue;
- }
}
// Now that we've canonicalized the condition to match the extend,
@@ -1542,22 +1521,6 @@ bool IndVarSimplify::canonicalizeExitCondition(Loop *L) {
// previously visible.
continue;
}
-
- // If we have a loop which would be undefined if infinite, and it has at
- // most one possible dynamic exit, then we can conclude that exit must
- // be taken. If that exit must be taken, and we know the LHS can only
- // take values in the positive domain, then we can conclude RHS must
- // also be in that same range.
- if (ExitingBlocks.size() == 1 && SE->loopHasNoAbnormalExits(L) &&
- SE->loopIsFiniteByAssumption(L)) {
- doRotateTransform();
- Changed = true;
- // Given we've changed exit counts, notify SCEV.
- // Some nested loops may share same folded exit basic block,
- // thus we need to notify top most loop.
- SE->forgetTopmostLoop(L);
- continue;
- }
}
return Changed;
diff --git a/llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll b/llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll
index 9a35a5b474dd..cc25d3e0f126 100644
--- a/llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll
+++ b/llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll
@@ -100,14 +100,13 @@ for.end: ; preds = %for.body, %entry
define void @slt_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @slt_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
-; CHECK-NEXT: [[UMAX:%.*]] = call i8 @llvm.umax.i8(i8 [[TMP0]], i8 1)
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[EXITCOND:%.*]] = icmp ne i8 [[IV_NEXT]], [[UMAX]]
-; CHECK-NEXT: br i1 [[EXITCOND]], label [[FOR_BODY]], label [[FOR_END:%.*]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp slt i16 [[ZEXT]], [[N:%.*]]
+; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
;
@@ -444,12 +443,12 @@ for.end: ; preds = %for.body, %entry
define void @sgt_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @sgt_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
@@ -498,12 +497,12 @@ for.end: ; preds = %for.body, %entry
define void @sle_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @sle_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ule i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp sle i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
@@ -552,12 +551,12 @@ for.end: ; preds = %for.body, %entry
define void @sge_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @sge_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp uge i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp sge i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
@@ -606,14 +605,13 @@ for.end: ; preds = %for.body, %entry
define void @ult_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @ult_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
-; CHECK-NEXT: [[UMAX:%.*]] = call i8 @llvm.umax.i8(i8 [[TMP0]], i8 1)
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[EXITCOND:%.*]] = icmp ne i8 [[IV_NEXT]], [[UMAX]]
-; CHECK-NEXT: br i1 [[EXITCOND]], label [[FOR_BODY]], label [[FOR_END:%.*]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp ult i16 [[ZEXT]], [[N:%.*]]
+; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
;
@@ -658,15 +656,42 @@ for.end: ; preds = %for.body, %entry
ret void
}
+define void @ugt_neg_non_loop(i16 %n.raw, i8 %start) mustprogress {
+; CHECK-LABEL: @ugt_neg_non_loop(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_BODY:%.*]]
+; CHECK: for.body:
+; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ [[START:%.*]], [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i16 [[ZEXT]], -2
+; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
+; CHECK: for.end:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %for.body
+
+for.body: ; preds = %entry, %for.body
+ %iv = phi i8 [ %iv.next, %for.body ], [ %start, %entry ]
+ %iv.next = add i8 %iv, 1
+ %zext = zext i8 %iv.next to i16
+ %cmp = icmp ugt i16 %zext, -2
+ br i1 %cmp, label %for.body, label %for.end
+
+for.end: ; preds = %for.body, %entry
+ ret void
+}
+
define void @ugt_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @ugt_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
@@ -715,12 +740,12 @@ for.end: ; preds = %for.body, %entry
define void @ule_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @ule_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ule i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp ule i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
@@ -769,12 +794,12 @@ for.end: ; preds = %for.body, %entry
define void @uge_non_constant_rhs(i16 %n) mustprogress {
; CHECK-LABEL: @uge_non_constant_rhs(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = trunc i16 [[N:%.*]] to i8
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[IV:%.*]] = phi i8 [ [[IV_NEXT:%.*]], [[FOR_BODY]] ], [ 0, [[ENTRY:%.*]] ]
; CHECK-NEXT: [[IV_NEXT]] = add i8 [[IV]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp uge i8 [[IV_NEXT]], [[TMP0]]
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
+; CHECK-NEXT: [[CMP:%.*]] = icmp uge i16 [[ZEXT]], [[N:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
; CHECK: for.end:
; CHECK-NEXT: ret void
_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
</body>
</html>