[indvars] Allow widenWithVariantUse to succeed without extend users #71557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were considering the IV to be potentially dead and returning as-if the transform had succeeded when there were no extend users. This is quite odd as there might be e.g. icmp users - in particular, icmp users which we've already provien we can widen. We can instead simply go ahead and perform the transform for all users, and avoid the need to retain the original IV.
|
@llvm/pr-subscribers-llvm-transforms Author: Philip Reames (preames) ChangesWe were considering the IV to be potentially dead and returning as-if the transform had succeeded when there were no extend users. This is quite odd as there might be e.g. icmp users - in particular, icmp users which we've already proven we can widen. We can instead simply go ahead and perform the transform for all users, and avoid the need to retain the original IV. Full diff: https://github.com/llvm/llvm-project/pull/71557.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
index 0b40d44c7443763..37735a0442ce205 100644
--- a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
@@ -1611,37 +1611,33 @@ bool WidenIV::widenWithVariantUse(WidenIV::NarrowIVDefUse DU) {
return false;
ExtUsers.push_back(User);
}
- if (ExtUsers.empty()) {
- DeadInsts.emplace_back(NarrowUse);
- return true;
- }
-
- // We'll prove some facts that should be true in the context of ext users. If
- // there is no users, we are done now. If there are some, pick their common
- // dominator as context.
- const Instruction *CtxI = findCommonDominator(ExtUsers, *DT);
-
- if (!CanSignExtend && !CanZeroExtend) {
- // Because InstCombine turns 'sub nuw' to 'add' losing the no-wrap flag, we
- // will most likely not see it. Let's try to prove it.
- if (OpCode != Instruction::Add)
- return false;
- if (ExtKind != ExtendKind::Zero)
- return false;
- const SCEV *LHS = SE->getSCEV(OBO->getOperand(0));
- const SCEV *RHS = SE->getSCEV(OBO->getOperand(1));
- // TODO: Support case for NarrowDef = NarrowUse->getOperand(1).
- if (NarrowUse->getOperand(0) != NarrowDef)
- return false;
- if (!SE->isKnownNegative(RHS))
- return false;
- bool ProvedSubNUW = SE->isKnownPredicateAt(ICmpInst::ICMP_UGE, LHS,
- SE->getNegativeSCEV(RHS), CtxI);
- if (!ProvedSubNUW)
- return false;
- // In fact, our 'add' is 'sub nuw'. We will need to widen the 2nd operand as
- // neg(zext(neg(op))), which is basically sext(op).
- AnotherOpExtKind = ExtendKind::Sign;
+ if (!ExtUsers.empty()) {
+ // We'll prove some facts that should be true in the context of ext users.
+ // Pick their common dominator as context.
+ const Instruction *CtxI = findCommonDominator(ExtUsers, *DT);
+
+ if (!CanSignExtend && !CanZeroExtend) {
+ // Because InstCombine turns 'sub nuw' to 'add' losing the no-wrap flag, we
+ // will most likely not see it. Let's try to prove it.
+ if (OpCode != Instruction::Add)
+ return false;
+ if (ExtKind != ExtendKind::Zero)
+ return false;
+ const SCEV *LHS = SE->getSCEV(OBO->getOperand(0));
+ const SCEV *RHS = SE->getSCEV(OBO->getOperand(1));
+ // TODO: Support case for NarrowDef = NarrowUse->getOperand(1).
+ if (NarrowUse->getOperand(0) != NarrowDef)
+ return false;
+ if (!SE->isKnownNegative(RHS))
+ return false;
+ bool ProvedSubNUW = SE->isKnownPredicateAt(ICmpInst::ICMP_UGE, LHS,
+ SE->getNegativeSCEV(RHS), CtxI);
+ if (!ProvedSubNUW)
+ return false;
+ // In fact, our 'add' is 'sub nuw'. We will need to widen the 2nd operand as
+ // neg(zext(neg(op))), which is basically sext(op).
+ AnotherOpExtKind = ExtendKind::Sign;
+ }
}
// Verifying that Defining operand is an AddRec
diff --git a/llvm/test/Transforms/IndVarSimplify/X86/pr57187.ll b/llvm/test/Transforms/IndVarSimplify/X86/pr57187.ll
index 620b0adaebfe584..7a3c829c3b0b628 100644
--- a/llvm/test/Transforms/IndVarSimplify/X86/pr57187.ll
+++ b/llvm/test/Transforms/IndVarSimplify/X86/pr57187.ll
@@ -15,14 +15,15 @@ define void @test(i32 %start) {
; CHECK-NEXT: br label [[LOOP]]
; CHECK: loop:
; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ [[INDVARS_IV_NEXT:%.*]], [[BACKEDGE:%.*]] ], [ [[TMP1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[TMP2:%.*]] = add nuw nsw i64 [[INDVARS_IV]], 4294967295
; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nsw i64 [[INDVARS_IV]], -1
-; CHECK-NEXT: [[INDVARS:%.*]] = trunc i64 [[INDVARS_IV_NEXT]] to i32
; CHECK-NEXT: [[LOOP_EXIT_COND:%.*]] = icmp slt i32 [[TMP0]], 11
; CHECK-NEXT: br i1 [[LOOP_EXIT_COND]], label [[EXIT:%.*]], label [[STUCK_PREHEADER:%.*]]
; CHECK: stuck.preheader:
; CHECK-NEXT: br label [[STUCK:%.*]]
; CHECK: exit:
-; CHECK-NEXT: [[IV_NEXT_LCSSA:%.*]] = phi i32 [ [[INDVARS]], [[LOOP]] ]
+; CHECK-NEXT: [[IV_NEXT_LCSSA_WIDE:%.*]] = phi i64 [ [[TMP2]], [[LOOP]] ]
+; CHECK-NEXT: [[TMP3:%.*]] = trunc i64 [[IV_NEXT_LCSSA_WIDE]] to i32
; CHECK-NEXT: ret void
; CHECK: stuck:
; CHECK-NEXT: br i1 false, label [[BACKEDGE]], label [[STUCK]]
diff --git a/llvm/test/Transforms/IndVarSimplify/widen-nonnegative-countdown.ll b/llvm/test/Transforms/IndVarSimplify/widen-nonnegative-countdown.ll
index d473103f5824e62..c9d84f1a5a63d24 100644
--- a/llvm/test/Transforms/IndVarSimplify/widen-nonnegative-countdown.ll
+++ b/llvm/test/Transforms/IndVarSimplify/widen-nonnegative-countdown.ll
@@ -82,13 +82,12 @@ define void @zext_postinc(ptr %A, i32 %start) {
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ [[TMP0]], [[FOR_BODY_PREHEADER]] ], [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY]] ]
-; CHECK-NEXT: [[J_016_US:%.*]] = phi i32 [ [[INC_US:%.*]], [[FOR_BODY]] ], [ [[START]], [[FOR_BODY_PREHEADER]] ]
; CHECK-NEXT: [[ARRAYIDX_US:%.*]] = getelementptr inbounds i32, ptr [[A:%.*]], i64 [[INDVARS_IV]]
; CHECK-NEXT: tail call void @use_ptr(ptr [[ARRAYIDX_US]])
-; CHECK-NEXT: [[INC_US]] = add nsw i32 [[J_016_US]], -1
-; CHECK-NEXT: [[CMP2_US:%.*]] = icmp ugt i32 [[INC_US]], 6
+; CHECK-NEXT: [[TMP1:%.*]] = add nsw i64 [[INDVARS_IV]], 4294967295
+; CHECK-NEXT: [[CMP2_US_WIDE:%.*]] = icmp ugt i64 [[TMP1]], 6
; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nsw i64 [[INDVARS_IV]], -1
-; CHECK-NEXT: br i1 [[CMP2_US]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
+; CHECK-NEXT: br i1 [[CMP2_US_WIDE]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
; CHECK: exit.loopexit:
; CHECK-NEXT: br label [[EXIT]]
; CHECK: exit:
@@ -365,16 +364,15 @@ define void @zext_postinc_offset_constant_one(ptr %A, i32 %start) {
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ [[TMP0]], [[FOR_BODY_PREHEADER]] ], [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY]] ]
-; CHECK-NEXT: [[J_016_US:%.*]] = phi i32 [ [[INC_US:%.*]], [[FOR_BODY]] ], [ [[START]], [[FOR_BODY_PREHEADER]] ]
; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[INDVARS_IV]] to i32
; CHECK-NEXT: [[ADD_US:%.*]] = add i32 [[TMP1]], 1
; CHECK-NEXT: [[IDXPROM_US:%.*]] = zext i32 [[ADD_US]] to i64
; CHECK-NEXT: [[ARRAYIDX_US:%.*]] = getelementptr inbounds i32, ptr [[A:%.*]], i64 [[IDXPROM_US]]
; CHECK-NEXT: tail call void @use_ptr(ptr [[ARRAYIDX_US]])
-; CHECK-NEXT: [[INC_US]] = add nsw i32 [[J_016_US]], -1
-; CHECK-NEXT: [[CMP2_US:%.*]] = icmp ugt i32 [[INC_US]], 6
+; CHECK-NEXT: [[TMP2:%.*]] = add nsw i64 [[INDVARS_IV]], 4294967295
+; CHECK-NEXT: [[CMP2_US_WIDE:%.*]] = icmp ugt i64 [[TMP2]], 6
; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nsw i64 [[INDVARS_IV]], -1
-; CHECK-NEXT: br i1 [[CMP2_US]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
+; CHECK-NEXT: br i1 [[CMP2_US_WIDE]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
; CHECK: exit.loopexit:
; CHECK-NEXT: br label [[EXIT]]
; CHECK: exit:
@@ -664,14 +662,13 @@ define void @zext_postinc_offset_constant_minus_one(ptr %A, i32 %start) {
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ [[TMP0]], [[FOR_BODY_PREHEADER]] ], [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY]] ]
-; CHECK-NEXT: [[J_016_US:%.*]] = phi i32 [ [[INC_US:%.*]], [[FOR_BODY]] ], [ [[START]], [[FOR_BODY_PREHEADER]] ]
; CHECK-NEXT: [[TMP1:%.*]] = add nsw i64 [[INDVARS_IV]], -1
; CHECK-NEXT: [[ARRAYIDX_US:%.*]] = getelementptr inbounds i32, ptr [[A:%.*]], i64 [[TMP1]]
; CHECK-NEXT: tail call void @use_ptr(ptr [[ARRAYIDX_US]])
-; CHECK-NEXT: [[INC_US]] = add nsw i32 [[J_016_US]], -1
-; CHECK-NEXT: [[CMP2_US:%.*]] = icmp ugt i32 [[INC_US]], 6
+; CHECK-NEXT: [[TMP2:%.*]] = add nsw i64 [[INDVARS_IV]], 4294967295
+; CHECK-NEXT: [[CMP2_US_WIDE:%.*]] = icmp ugt i64 [[TMP2]], 6
; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nsw i64 [[INDVARS_IV]], -1
-; CHECK-NEXT: br i1 [[CMP2_US]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
+; CHECK-NEXT: br i1 [[CMP2_US_WIDE]], label [[FOR_BODY]], label [[EXIT_LOOPEXIT:%.*]]
; CHECK: exit.loopexit:
; CHECK-NEXT: br label [[EXIT]]
; CHECK: exit:
diff --git a/llvm/test/Transforms/LoopFlatten/widen-iv3.ll b/llvm/test/Transforms/LoopFlatten/widen-iv3.ll
index df8ee6ff0750574..12c30c271f07df4 100644
--- a/llvm/test/Transforms/LoopFlatten/widen-iv3.ll
+++ b/llvm/test/Transforms/LoopFlatten/widen-iv3.ll
@@ -34,11 +34,13 @@ define i16 @foo() {
; CHECK-NEXT: [[J_011:%.*]] = phi i16 [ 0, [[FOR_COND1_PREHEADER]] ], [ [[INC:%.*]], [[FOR_BODY4]] ]
; CHECK-NEXT: [[SUM_110:%.*]] = phi i16 [ [[SUM_012]], [[FOR_COND1_PREHEADER]] ], [ [[ADD5]], [[FOR_BODY4]] ]
; CHECK-NEXT: [[TMP2:%.*]] = add nuw nsw i32 [[INDVAR]], [[TMP0]]
+; CHECK-NEXT: [[TMP3:%.*]] = sext i16 [[J_011]] to i32
+; CHECK-NEXT: [[TMP4:%.*]] = add nuw nsw i32 [[TMP3]], [[TMP0]]
; CHECK-NEXT: [[ADD:%.*]] = add nuw nsw i16 [[J_011]], [[MUL]]
-; CHECK-NEXT: [[TMP3:%.*]] = trunc i32 [[TMP2]] to i16
-; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [64 x i16], ptr @v, i16 0, i16 [[TMP3]]
-; CHECK-NEXT: [[TMP4:%.*]] = load i16, ptr [[ARRAYIDX]], align 1
-; CHECK-NEXT: [[ADD5]] = add nsw i16 [[TMP4]], [[SUM_110]]
+; CHECK-NEXT: [[TMP5:%.*]] = trunc i32 [[TMP2]] to i16
+; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [64 x i16], ptr @v, i16 0, i16 [[TMP5]]
+; CHECK-NEXT: [[TMP6:%.*]] = load i16, ptr [[ARRAYIDX]], align 1
+; CHECK-NEXT: [[ADD5]] = add nsw i16 [[TMP6]], [[SUM_110]]
; CHECK-NEXT: [[INDVAR_NEXT]] = add i32 [[INDVAR]], 1
; CHECK-NEXT: [[INC]] = add nuw nsw i16 [[J_011]], 1
; CHECK-NEXT: [[EXITCOND_NOT:%.*]] = icmp eq i32 [[INDVAR_NEXT]], 16
|
You can test this locally with the following command:git-clang-format --diff 75d6795e420274346b14aca8b6bd49bfe6030eeb 616ab41a7a325a8c6160abae2e09ade94d0e41ad -- llvm/lib/Transforms/Utils/SimplifyIndVar.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
index 37735a0442ce..006dcd8339a6 100644
--- a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
@@ -1617,8 +1617,8 @@ bool WidenIV::widenWithVariantUse(WidenIV::NarrowIVDefUse DU) {
const Instruction *CtxI = findCommonDominator(ExtUsers, *DT);
if (!CanSignExtend && !CanZeroExtend) {
- // Because InstCombine turns 'sub nuw' to 'add' losing the no-wrap flag, we
- // will most likely not see it. Let's try to prove it.
+ // Because InstCombine turns 'sub nuw' to 'add' losing the no-wrap flag,
+ // we will most likely not see it. Let's try to prove it.
if (OpCode != Instruction::Add)
return false;
if (ExtKind != ExtendKind::Zero)
@@ -1630,12 +1630,12 @@ bool WidenIV::widenWithVariantUse(WidenIV::NarrowIVDefUse DU) {
return false;
if (!SE->isKnownNegative(RHS))
return false;
- bool ProvedSubNUW = SE->isKnownPredicateAt(ICmpInst::ICMP_UGE, LHS,
- SE->getNegativeSCEV(RHS), CtxI);
+ bool ProvedSubNUW = SE->isKnownPredicateAt(
+ ICmpInst::ICMP_UGE, LHS, SE->getNegativeSCEV(RHS), CtxI);
if (!ProvedSubNUW)
return false;
- // In fact, our 'add' is 'sub nuw'. We will need to widen the 2nd operand as
- // neg(zext(neg(op))), which is basically sext(op).
+ // In fact, our 'add' is 'sub nuw'. We will need to widen the 2nd operand
+ // as neg(zext(neg(op))), which is basically sext(op).
AnotherOpExtKind = ExtendKind::Sign;
}
}
|
nikic
reviewed
Nov 8, 2023
| @@ -34,11 +34,13 @@ define i16 @foo() { | |||
| ; CHECK-NEXT: [[J_011:%.*]] = phi i16 [ 0, [[FOR_COND1_PREHEADER]] ], [ [[INC:%.*]], [[FOR_BODY4]] ] | |||
| ; CHECK-NEXT: [[SUM_110:%.*]] = phi i16 [ [[SUM_012]], [[FOR_COND1_PREHEADER]] ], [ [[ADD5]], [[FOR_BODY4]] ] | |||
| ; CHECK-NEXT: [[TMP2:%.*]] = add nuw nsw i32 [[INDVAR]], [[TMP0]] | |||
| ; CHECK-NEXT: [[TMP3:%.*]] = sext i16 [[J_011]] to i32 | |||
| ; CHECK-NEXT: [[TMP4:%.*]] = add nuw nsw i32 [[TMP3]], [[TMP0]] | |||
There was a problem hiding this comment.
These instructions look dead. Is that the fault of LoopFlatten or IndVars?
There was a problem hiding this comment.
LoopFlatten. It doesn't call up after calling the widening routines. I added adce to at least one of the LoopFlatten test previously, can do so for this one as well. (They're already invoking multiple passes.)
There was a problem hiding this comment.
I see, LoopFlatten ignores the DeadInsts parameter. It would be best to make it drop the dead insts directly instead of adding adce to tests.
| ; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[INDVARS_IV]] to i32 | ||
| ; CHECK-NEXT: [[ADD_US:%.*]] = add i32 [[TMP1]], 1 | ||
| ; CHECK-NEXT: [[IDXPROM_US:%.*]] = zext i32 [[ADD_US]] to i64 | ||
| ; CHECK-NEXT: [[ARRAYIDX_US:%.*]] = getelementptr inbounds i32, ptr [[A:%.*]], i64 [[IDXPROM_US]] | ||
| ; CHECK-NEXT: tail call void @use_ptr(ptr [[ARRAYIDX_US]]) | ||
| ; CHECK-NEXT: [[INC_US]] = add nsw i32 [[J_016_US]], -1 | ||
| ; CHECK-NEXT: [[CMP2_US:%.*]] = icmp ugt i32 [[INC_US]], 6 | ||
| ; CHECK-NEXT: [[TMP2:%.*]] = add nsw i64 [[INDVARS_IV]], 4294967295 |
There was a problem hiding this comment.
I don't really get why this transform is correct. It looks like indvars.iv here is a normal {%start,+,-1} addrec, but then the exit condition here becomes {%start+4294967295,+,-1} ugt 6? That's not the same as {%start-1,+,-1} ugt 6 in a smaller bit width.
Am I missing something here?
There was a problem hiding this comment.
The key fact is that we have a loop entry condition which proves both that start is non-negative, and that start -1 is still positive. Given this precondition trunc(zext(start)+4294967295) == start-1. Given we know we exit without the IV every becoming negative (in either 32 or 64), this should be the same.
In general, this transform relies on the high bits of the addrec being don't care except for the identified users it visits and proves that the result of the user is unchanged by the possible change in the high bits.
There was a problem hiding this comment.
Actually, hold on. You're right and there's something funky going on here. We have a 64 bit IV which enumerates the same values as the 32 bit IV, but then the actual comparison uses a 64 bit offset (without truncation), and the high bits change the exit condition.
I need to investigate this further, thanks for spotting.
|
No longer actively working on this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were considering the IV to be potentially dead and returning as-if the transform had succeeded when there were no extend users. This is quite odd as there might be e.g. icmp users - in particular, icmp users which we've already proven we can widen. We can instead simply go ahead and perform the transform for all users, and avoid the need to retain the original IV.