Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCEVExpander] Don't try to reuse SCEVUnknown values #115141

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Nov 6, 2024

The expansion of a SCEVUnknown is trivial (it's just the wrapped value). If we try to reuse an existing value it might be a more complex expression that simplifies to the SCEVUnknown.

This is inspired by #114879, because SCEVExpander replacing a constant with a phi node is just silly. (I don't consider this a fix for that issue though.)

@llvmbot
Copy link
Member

llvmbot commented Nov 6, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

The expansion of a SCEVUnknown is trivial (it's just the wrapped value). If we try to reuse an existing value it might be a more complex expression that simplifies to the SCEVUnknown.

This is inspired by #114879, because SCEVExpander replacing a constant with a phi node is just silly. (I don't consider this a fix for that issue though.)


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

4 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp (+2-2)
  • (modified) llvm/test/Transforms/LoopStrengthReduce/Power/incomplete-phi.ll (+1-3)
  • (modified) llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll (+2-2)
  • (modified) llvm/test/Transforms/LoopVectorize/pr45259.ll (+5-3)
diff --git a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
index 39da38e4918176..791d528823972d 100644
--- a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -1467,8 +1467,8 @@ Value *SCEVExpander::FindValueInExprValueMap(
   if (!CanonicalMode && SE.containsAddRecurrence(S))
     return nullptr;
 
-  // If S is a constant, it may be worse to reuse an existing Value.
-  if (isa<SCEVConstant>(S))
+  // If S is a constant or unknown, it may be worse to reuse an existing Value.
+  if (isa<SCEVConstant>(S) || isa<SCEVUnknown>(S))
     return nullptr;
 
   for (Value *V : SE.getSCEVValues(S)) {
diff --git a/llvm/test/Transforms/LoopStrengthReduce/Power/incomplete-phi.ll b/llvm/test/Transforms/LoopStrengthReduce/Power/incomplete-phi.ll
index c57761c1a01f14..53aac1d9cf7f8d 100644
--- a/llvm/test/Transforms/LoopStrengthReduce/Power/incomplete-phi.ll
+++ b/llvm/test/Transforms/LoopStrengthReduce/Power/incomplete-phi.ll
@@ -21,12 +21,10 @@ define void @foo(ptr %arg) {
 ; CHECK-LABEL: define void @foo(
 ; CHECK-SAME: ptr [[ARG:%.*]]) {
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:    [[I:%.*]] = getelementptr [0 x %0], ptr [[ARG]], i64 0, i64 -1
-; CHECK-NEXT:    [[I2:%.*]] = getelementptr i8, ptr [[I]], i64 4
 ; CHECK-NEXT:    [[SCEVGEP:%.*]] = getelementptr i8, ptr [[ARG]], i64 396
 ; CHECK-NEXT:    br label [[BB3:%.*]]
 ; CHECK:       bb3:
-; CHECK-NEXT:    [[LSR_IV7:%.*]] = phi ptr [ [[SCEVGEP8:%.*]], [[BB18:%.*]] ], [ [[I2]], [[BB:%.*]] ]
+; CHECK-NEXT:    [[LSR_IV7:%.*]] = phi ptr [ [[SCEVGEP8:%.*]], [[BB18:%.*]] ], [ [[ARG]], [[BB:%.*]] ]
 ; CHECK-NEXT:    [[LSR_IV5:%.*]] = phi i64 [ [[LSR_IV_NEXT6:%.*]], [[BB18]] ], [ 4, [[BB]] ]
 ; CHECK-NEXT:    [[LSR_IV1:%.*]] = phi ptr [ [[SCEVGEP2:%.*]], [[BB18]] ], [ [[SCEVGEP]], [[BB]] ]
 ; CHECK-NEXT:    br i1 true, label [[BB22_PREHEADER:%.*]], label [[BB9_PREHEADER:%.*]]
diff --git a/llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll b/llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll
index 6f34dc843ae1ee..3c53befa67e230 100644
--- a/llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll
+++ b/llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll
@@ -113,7 +113,7 @@ for.end:                                          ; preds = %for.body
 define void @ptr_of_ptr_addrec(ptr %ptrptr, i32 %length) {
 ; CHECK-LABEL: @ptr_of_ptr_addrec(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[START_PTRPTR:%.*]] = getelementptr ptr, ptr [[PTRPTR:%.*]]
+; CHECK-NEXT:    [[START_PTRPTR1:%.*]] = getelementptr inbounds ptr, ptr [[START_PTRPTR:%.*]]
 ; CHECK-NEXT:    [[TMP0:%.*]] = add i32 [[LENGTH:%.*]], -1
 ; CHECK-NEXT:    [[TMP1:%.*]] = zext i32 [[TMP0]] to i64
 ; CHECK-NEXT:    [[TMP2:%.*]] = shl nuw nsw i64 [[TMP1]], 3
@@ -121,7 +121,7 @@ define void @ptr_of_ptr_addrec(ptr %ptrptr, i32 %length) {
 ; CHECK-NEXT:    [[SCEVGEP:%.*]] = getelementptr i8, ptr [[START_PTRPTR]], i64 [[TMP3]]
 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
 ; CHECK:       for.body:
-; CHECK-NEXT:    [[IT_04:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], [[FOR_BODY]] ], [ [[START_PTRPTR]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[IT_04:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], [[FOR_BODY]] ], [ [[START_PTRPTR1]], [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    [[TMP4:%.*]] = load ptr, ptr [[IT_04]], align 8
 ; CHECK-NEXT:    tail call void @foo(ptr [[TMP4]])
 ; CHECK-NEXT:    [[INCDEC_PTR]] = getelementptr ptr, ptr [[IT_04]], i64 1
diff --git a/llvm/test/Transforms/LoopVectorize/pr45259.ll b/llvm/test/Transforms/LoopVectorize/pr45259.ll
index 008971697775e4..9a154709aab813 100644
--- a/llvm/test/Transforms/LoopVectorize/pr45259.ll
+++ b/llvm/test/Transforms/LoopVectorize/pr45259.ll
@@ -14,12 +14,14 @@ define i8 @widget(ptr %arr, i8 %t9) {
 ; CHECK-NEXT:    br i1 [[C]], label [[FOR_PREHEADER:%.*]], label [[BB6]]
 ; CHECK:       for.preheader:
 ; CHECK-NEXT:    [[T1_0_LCSSA:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
+; CHECK-NEXT:    [[T1_0_LCSSA4:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
+; CHECK-NEXT:    [[T1_0_LCSSA1:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
 ; CHECK-NEXT:    [[TMP0:%.*]] = trunc i64 [[ARR1]] to i32
 ; CHECK-NEXT:    [[TMP1:%.*]] = sub i32 0, [[TMP0]]
 ; CHECK-NEXT:    [[T1_0_LCSSA3:%.*]] = ptrtoint ptr [[T1_0_LCSSA]] to i64
 ; CHECK-NEXT:    [[TMP2:%.*]] = trunc i64 [[T1_0_LCSSA3]] to i32
 ; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP1]], [[TMP2]]
-; CHECK-NEXT:    [[T1_0_LCSSA2:%.*]] = ptrtoint ptr [[T1_0_LCSSA]] to i64
+; CHECK-NEXT:    [[T1_0_LCSSA2:%.*]] = ptrtoint ptr [[T1_0_LCSSA4]] to i64
 ; CHECK-NEXT:    [[MIN_ITERS_CHECK:%.*]] = icmp ult i32 [[TMP3]], 4
 ; CHECK-NEXT:    br i1 [[MIN_ITERS_CHECK]], label [[SCALAR_PH:%.*]], label [[VECTOR_SCEVCHECK:%.*]]
 ; CHECK:       vector.scevcheck:
@@ -65,8 +67,8 @@ define i8 @widget(ptr %arr, i8 %t9) {
 ; CHECK-NEXT:    [[T3_I:%.*]] = icmp slt i8 [[IV_NEXT]], [[T9]]
 ; CHECK-NEXT:    [[T3_I8:%.*]] = zext i1 [[T3_I]] to i8
 ; CHECK-NEXT:    store i8 [[T3_I8]], ptr [[PTR]], align 1
-; CHECK-NEXT:    [[EC:%.*]] = icmp eq ptr [[T1_0_LCSSA]], [[PTR]]
-; CHECK-NEXT:    br i1 [[EC]], label [[FOR_EXIT]], label [[FOR_BODY]], !llvm.loop [[LOOP2:![0-9]+]]
+; CHECK-NEXT:    [[EC:%.*]] = icmp eq ptr [[T1_0_LCSSA1]], [[PTR]]
+; CHECK-NEXT:    br i1 [[EC]], label [[FOR_EXIT]], label [[FOR_BODY]], !llvm.loop [[LOOP3:![0-9]+]]
 ; CHECK:       for.exit:
 ; CHECK-NEXT:    [[IV_NEXT_LCSSA:%.*]] = phi i8 [ [[IV_NEXT]], [[FOR_BODY]] ], [ [[IND_END]], [[MIDDLE_BLOCK]] ]
 ; CHECK-NEXT:    ret i8 [[IV_NEXT_LCSSA]]

@@ -14,12 +14,14 @@ define i8 @widget(ptr %arr, i8 %t9) {
; CHECK-NEXT: br i1 [[C]], label [[FOR_PREHEADER:%.*]], label [[BB6]]
; CHECK: for.preheader:
; CHECK-NEXT: [[T1_0_LCSSA:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
; CHECK-NEXT: [[T1_0_LCSSA4:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
; CHECK-NEXT: [[T1_0_LCSSA1:%.*]] = phi ptr [ [[T1_0]], [[BB6]] ]
Copy link
Contributor Author

@nikic nikic Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downside is that we might generate more duplicate LCSSA phis, but this seems like an independent SCEVExpander problem.

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. No changes on real-world code.

The expansion of a SCEVUnknown is trivial (it's just the wrapped
value). If we try to reuse an existing value it might be a more
complex expression that simplifies to the SCEVUnknown.
@nikic nikic force-pushed the scevexpander-unknown-reuse branch from 5bee988 to 547c561 Compare November 7, 2024 09:55
@nikic nikic merged commit 6dc23b7 into llvm:main Nov 11, 2024
8 checks passed
@nikic nikic deleted the scevexpander-unknown-reuse branch November 11, 2024 11:36
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
The expansion of a SCEVUnknown is trivial (it's just the wrapped value).
If we try to reuse an existing value it might be a more complex
expression that simplifies to the SCEVUnknown.

This is inspired by llvm#114879,
because SCEVExpander replacing a constant with a phi node is just silly.
(I don't consider this a fix for that issue though.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants