[llvm] d4708fa - Backout must-exit based parts of 3fc9882e, and 412eb0
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 4 16:00:44 PDT 2021
Ok, mostly for history preservation sake...
As I'd expected yesterday, the reverted code was incorrect. Essentially,
the error I'd made is that when I ported the logic from 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.
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.
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.
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.
Philip
On 11/3/21 3:20 PM, Philip Reames via llvm-commits wrote:
> Author: Philip Reames
> Date: 2021-11-03T15:19:49-07:00
> New Revision: d4708fa480f2894f3cc007cbcb8aaf2ad754d34b
>
> URL: https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b
> DIFF: https://github.com/llvm/llvm-project/commit/d4708fa480f2894f3cc007cbcb8aaf2ad754d34b.diff
>
> 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
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211104/4c34f7a8/attachment-0001.html>
More information about the llvm-commits
mailing list