[SCEV] Simplify SCEVExpr for PHI to SCEV for operand if operands are identical #115945
Conversation
|
@llvm/pr-subscribers-llvm-analysis Author: Akshay Deodhar (akshayrdeodhar) ChangesHelps SCEV analyze some special phi nodes, allowing the computation of loop trip count in cases like the following: https://godbolt.org/z/xGs1d81TW Full diff: https://github.com/llvm/llvm-project/pull/115945.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/ScalarEvolution.h b/llvm/include/llvm/Analysis/ScalarEvolution.h
index 4b8cb3a39a86db..86f4cb92152836 100644
--- a/llvm/include/llvm/Analysis/ScalarEvolution.h
+++ b/llvm/include/llvm/Analysis/ScalarEvolution.h
@@ -1761,6 +1761,10 @@ class ScalarEvolution {
/// V.
const SCEV *getOperandsToCreate(Value *V, SmallVectorImpl<Value *> &Ops);
+ /// Returns SCEV for the first operand of a phi if all phi operands have
+ /// identical opcodes and operands
+ const SCEV *createNodeForPHIWithIdenticalOperands(PHINode *PN);
+
/// Provide the special handling we need to analyze PHI SCEVs.
const SCEV *createNodeForPHI(PHINode *PN);
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index b10811133770e1..3480dda484f333 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -6019,6 +6019,37 @@ const SCEV *ScalarEvolution::createNodeFromSelectLikePHI(PHINode *PN) {
return nullptr;
}
+// Returns SCEV for the first operand of a phi if all phi operands have
+// identical opcodes and operands
+// eg.
+// a: %add = %a + %b
+// br %c
+// b: %add1 = %a + %b
+// br %c
+// c: %phi = phi [%add, a], [%add1, b]
+// scev(%phi) => scev(%add)
+const SCEV *
+ScalarEvolution::createNodeForPHIWithIdenticalOperands(PHINode *PN) {
+ BinaryOperator *CommonInst = nullptr;
+ for (Value *Incoming : PN->incoming_values()) {
+ BinaryOperator *IncomingInst = dyn_cast<BinaryOperator>(Incoming);
+ if (CommonInst) {
+ if (!(IncomingInst && CommonInst->isIdenticalTo(IncomingInst))) {
+ // Not identical, give up
+ CommonInst = nullptr;
+ break;
+ }
+ } else if (IncomingInst) {
+ // Remember binary operator
+ CommonInst = IncomingInst;
+ } else {
+ // Not a binary operator, give up
+ return nullptr;
+ }
+ }
+ return CommonInst ? getSCEV(CommonInst) : nullptr;
+}
+
const SCEV *ScalarEvolution::createNodeForPHI(PHINode *PN) {
if (const SCEV *S = createAddRecFromPHI(PN))
return S;
@@ -6030,6 +6061,9 @@ const SCEV *ScalarEvolution::createNodeForPHI(PHINode *PN) {
/*UseInstrInfo=*/true, /*CanUseUndef=*/false}))
return getSCEV(V);
+ if (const SCEV *S = createNodeForPHIWithIdenticalOperands(PN))
+ return S;
+
if (const SCEV *S = createNodeFromSelectLikePHI(PN))
return S;
diff --git a/llvm/test/Analysis/ScalarEvolution/trip-count16.ll b/llvm/test/Analysis/ScalarEvolution/trip-count16.ll
new file mode 100644
index 00000000000000..2b267edb2438f9
--- /dev/null
+++ b/llvm/test/Analysis/ScalarEvolution/trip-count16.ll
@@ -0,0 +1,46 @@
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -disable-output "-passes=print<scalar-evolution>" 2>&1 | FileCheck %s
+define void @test(ptr %x, ptr %y) {
+; CHECK-LABEL: 'test'
+; CHECK-NEXT: Classifying expressions for: @test
+; CHECK-NEXT: %v1.0 = phi i32 [ 0, %entry ], [ %k.0, %if.end ]
+; CHECK-NEXT: --> {0,+,1}<nuw><nsw><%for.cond> U: [0,7) S: [0,7) Exits: 6 LoopDispositions: { %for.cond: Computable }
+; CHECK-NEXT: %add = add nsw i32 %v1.0, 1
+; CHECK-NEXT: --> {1,+,1}<nuw><nsw><%for.cond> U: [1,8) S: [1,8) Exits: 7 LoopDispositions: { %for.cond: Computable }
+; CHECK-NEXT: %add6 = add nsw i32 %v1.0, 1
+; CHECK-NEXT: --> {1,+,1}<nuw><nsw><%for.cond> U: [1,8) S: [1,8) Exits: 7 LoopDispositions: { %for.cond: Computable }
+; CHECK-NEXT: %k.0 = phi i32 [ %add, %if.then ], [ %add6, %if.else ]
+; CHECK-NEXT: --> {1,+,1}<nuw><nsw><%for.cond> U: [1,8) S: [1,8) Exits: 7 LoopDispositions: { %for.cond: Computable }
+; CHECK-NEXT: Determining loop execution counts for: @test
+; CHECK-NEXT: Loop %for.cond: backedge-taken count is i32 6
+; CHECK-NEXT: Loop %for.cond: constant max backedge-taken count is i32 6
+; CHECK-NEXT: Loop %for.cond: symbolic max backedge-taken count is i32 6
+; CHECK-NEXT: Loop %for.cond: Trip multiple is 7
+;
+entry:
+ br label %for.cond
+
+for.cond: ; preds = %6, %0
+ %v1.0 = phi i32 [ 0, %entry ], [ %k.0, %if.end ]
+ %cmp = icmp slt i32 %v1.0, 6
+ br i1 %cmp, label %for.body, label %exit
+
+for.body: ; preds = %1
+ %cmp3 = icmp slt i32 %v1.0, 2
+ br i1 %cmp3, label %if.then, label %if.else
+
+if.then: ; preds = %2
+ %add = add nsw i32 %v1.0, 1
+ br label %if.end
+
+if.else: ; preds = %2
+ %add6 = add nsw i32 %v1.0, 1
+ br label %if.end
+
+if.end: ; preds = %4, %3
+ %k.0 = phi i32 [ %add, %if.then ], [ %add6, %if.else ]
+ br label %for.cond
+
+exit: ; preds = %5
+ ret void
+}
|
akshayrdeodhar
requested review from
fhahn,
nikic and
dtcxzyw
and removed request for
nikic
|
CC: @fiigii |
| for (Value *Incoming : PN->incoming_values()) { | ||
| BinaryOperator *IncomingInst = dyn_cast<BinaryOperator>(Incoming); | ||
| if (CommonInst) { | ||
| if (!(IncomingInst && CommonInst->isIdenticalTo(IncomingInst))) { |
There was a problem hiding this comment.
Should we check that all incoming instructions belong to the same loop?
There was a problem hiding this comment.
If the operands of the phi are identical, then the simplification is correct regardless of whether instructions belong to the same loop- I think there's no advantage to special-casing this.
akshayrdeodhar
requested review from
cyndyishida,
rupprecht,
keith,
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci,
ftynse,
makslevental,
stellaraccident,
grypp,
aartbik,
PeimingLiu and
yinying-lisa-li
as code owners
|
Erroneously pushed a partial rebase, reviewers were added due to merge conflicts. Have removed them now, and have restored the PR to be based off an older version of main. |
There was a problem hiding this comment.
This looks somewhat iffy -- SCEV really shouldn't be going this kind of instruction comparison, that's the job of instruction hoisting. Of course, we have some weaknesses that prevent it from working in your example. I believe #78615 would fix it, but it's a tricky change. If this is common/important in practice, working around the issue in SCEV is an option.
| return nullptr; | ||
| } | ||
| } | ||
| return CommonInst ? getSCEV(CommonInst) : nullptr; |
There was a problem hiding this comment.
This looks dangerous to me. Even if the instructions are the same, I think that SCEV would be allowed to use context-sensitive reasoning when constructing the SCEV node. It would be safer to call getSCEV for each incoming value in a second loop and make sure they're all the same.
There was a problem hiding this comment.
Using a single loop with calls to getSCEV.
There was a problem hiding this comment.
I expect that you do need both loops -- not for correctness, but as a profitability heuristic. Computing SCEVs is expensive, and always recursing through phis would likely add significant cost.
I tried to confirm this but the stage2 build crashes (https://llvm-compile-time-tracker.com/show_error.php?commit=a4e3a0e648d7a3664ca6269e846abf2e6ddcdb08). I didn't investigate, but it might be that the recursion causes a stack overflow.
There was a problem hiding this comment.
Restored the original loop, and added an all_of check for SCEV exprs of the incoming values being identical.
|
Thanks for the suggestions! Won't have internet access for 2 days, will address the comments on Monday. This does affect some real workloads, which is why I wanted to work around this in SCEV. |
There was a problem hiding this comment.
Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=29062329f3cf0ac8f1ae626e758ca64f82294fbf&to=b49b86076b0cfe3449462cc8e81fd2e450d0088d&stat=instructions%3Au There is a large impact on kimwy.cc, but it also has large code size changes, so probably second order effects.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Not sure if this regression matters... Regression (reduced from https://github.com/dtcxzyw/llvm-opt-benchmark/pull/1696/files#r1843118408): Before (22417ec): After: |
|
Sorry, missed your comment (dtcxzyw/llvm-opt-benchmark#1696 (comment)) on the other MR. Am looking into the reduced case. |
|
Looked into this- this is expected behaviour, I think we should ignore this regression. Here's the debug output of that pass, for the changed version of the compiler: Before the change, the compiler fails to do IV widening. This is because the new compiler is able to create an AddRecExpr for |
Can we widen |
This will be an improvement to IndVarSimplify- I'm not familiar with that pass - will it be fine if I land this MR independently of the improvement? |
Yeah. I don't mean to block this MR. |
|
Thanks for the reviews! |
…identical (llvm#115945) Helps SCEV analyze some special phi nodes, allowing the computation of loop trip count in cases like the following: https://godbolt.org/z/xGs1d81TW
…identical (llvm#115945) Helps SCEV analyze some special phi nodes, allowing the computation of loop trip count in cases like the following: https://godbolt.org/z/xGs1d81TW
Helps SCEV analyze some special phi nodes, allowing the computation of loop trip count in cases like the following:
https://godbolt.org/z/xGs1d81TW