release/18.x: [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (#91215) #91419

AtariDreams · 2024-05-08T02:23:31Z

See the LangRef:

All uses of a value returned by the same ‘freeze’ instruction are
guaranteed to always observe the same value, while different ‘freeze’ instructions may yield different values.

It is incorrect to replace freezes with the simplified value.

Proof:
https://alive2.llvm.org/ce/z/3Dn9Cd
https://alive2.llvm.org/ce/z/Qyh5h6

Fixes #91178

(cherry picked from commit d085b42)

llvmbot · 2024-05-08T02:24:02Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: AtariDreams (AtariDreams)

Changes

See the LangRef:
> All uses of a value returned by the same ‘freeze’ instruction are
guaranteed to always observe the same value, while different ‘freeze’ instructions may yield different values.

It is incorrect to replace freezes with the simplified value.

Proof:
https://alive2.llvm.org/ce/z/3Dn9Cd
https://alive2.llvm.org/ce/z/Qyh5h6

Fixes #91178

(cherry picked from commit d085b42)

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

4 Files Affected:

(modified) llvm/lib/Analysis/InstructionSimplify.cpp (+4)
(modified) llvm/test/Transforms/InstCombine/icmp.ll (+15)
(modified) llvm/test/Transforms/InstCombine/select.ll (+88)
(modified) llvm/test/Transforms/PGOProfile/chr.ll (+4-3)

diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 72b6dfa181e86..8dcffe45c644b 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -4322,6 +4322,10 @@ static Value *simplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
   if (match(I, m_Intrinsic<Intrinsic::is_constant>()))
     return nullptr;
 
+  // Don't simplify freeze.
+  if (isa<FreezeInst>(I))
+    return nullptr;
+
   // Replace Op with RepOp in instruction operands.
   SmallVector<Value *, 8> NewOps;
   bool AnyReplaced = false;
diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll
index 10ab1fe118348..9ac35745742bb 100644
--- a/llvm/test/Transforms/InstCombine/icmp.ll
+++ b/llvm/test/Transforms/InstCombine/icmp.ll
@@ -5183,3 +5183,18 @@ entry:
   %cmp = icmp eq i8 %add2, %add1
   ret i1 %cmp
 }
+
+define i1 @icmp_freeze_sext(i16 %x, i16 %y) {
+; CHECK-LABEL: @icmp_freeze_sext(
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp uge i16 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[CMP1_FR:%.*]] = freeze i1 [[CMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i16 [[Y]], 0
+; CHECK-NEXT:    [[CMP2:%.*]] = or i1 [[TMP1]], [[CMP1_FR]]
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  %cmp1 = icmp uge i16 %x, %y
+  %ext = sext i1 %cmp1 to i16
+  %ext.fr = freeze i16 %ext
+  %cmp2 = icmp uge i16 %ext.fr, %y
+  ret i1 %cmp2
+}
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index 888e7d28f78af..111ac62986d4d 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -3708,3 +3708,91 @@ define i32 @src_select_xxory_eq0_xorxy_y(i32 %x, i32 %y) {
   %cond = select i1 %xor0, i32 %xor, i32 %y
   ret i32 %cond
 }
+
+define i32 @sequence_select_with_same_cond_false(i1 %c1, i1 %c2){
+; CHECK-LABEL: @sequence_select_with_same_cond_false(
+; CHECK-NEXT:    [[S1:%.*]] = select i1 [[C1:%.*]], i32 23, i32 45
+; CHECK-NEXT:    [[S2:%.*]] = select i1 [[C2:%.*]], i32 666, i32 [[S1]]
+; CHECK-NEXT:    [[S3:%.*]] = select i1 [[C1]], i32 789, i32 [[S2]]
+; CHECK-NEXT:    ret i32 [[S3]]
+;
+  %s1 = select i1 %c1, i32 23, i32 45
+  %s2 = select i1 %c2, i32 666, i32 %s1
+  %s3 = select i1 %c1, i32 789, i32 %s2
+  ret i32 %s3
+}
+
+define i32 @sequence_select_with_same_cond_true(i1 %c1, i1 %c2){
+; CHECK-LABEL: @sequence_select_with_same_cond_true(
+; CHECK-NEXT:    [[S1:%.*]] = select i1 [[C1:%.*]], i32 45, i32 23
+; CHECK-NEXT:    [[S2:%.*]] = select i1 [[C2:%.*]], i32 [[S1]], i32 666
+; CHECK-NEXT:    [[S3:%.*]] = select i1 [[C1]], i32 [[S2]], i32 789
+; CHECK-NEXT:    ret i32 [[S3]]
+;
+  %s1 = select i1 %c1, i32 45, i32 23
+  %s2 = select i1 %c2, i32 %s1, i32 666
+  %s3 = select i1 %c1, i32 %s2, i32 789
+  ret i32 %s3
+}
+
+define double @sequence_select_with_same_cond_double(double %a, i1 %c1, i1 %c2, double %r1, double %r2){
+; CHECK-LABEL: @sequence_select_with_same_cond_double(
+; CHECK-NEXT:    [[S1:%.*]] = select i1 [[C1:%.*]], double 1.000000e+00, double 0.000000e+00
+; CHECK-NEXT:    [[S2:%.*]] = select i1 [[C2:%.*]], double [[S1]], double 2.000000e+00
+; CHECK-NEXT:    [[S3:%.*]] = select i1 [[C1]], double [[S2]], double 3.000000e+00
+; CHECK-NEXT:    ret double [[S3]]
+;
+  %s1 = select i1 %c1, double 1.0, double 0.0
+  %s2 = select i1 %c2, double %s1, double 2.0
+  %s3 = select i1 %c1, double %s2, double 3.0
+  ret double %s3
+}
+
+declare void @use32(i32)
+
+define i32 @sequence_select_with_same_cond_extra_use(i1 %c1, i1 %c2){
+; CHECK-LABEL: @sequence_select_with_same_cond_extra_use(
+; CHECK-NEXT:    [[S1:%.*]] = select i1 [[C1:%.*]], i32 23, i32 45
+; CHECK-NEXT:    call void @use32(i32 [[S1]])
+; CHECK-NEXT:    [[S2:%.*]] = select i1 [[C2:%.*]], i32 666, i32 [[S1]]
+; CHECK-NEXT:    [[S3:%.*]] = select i1 [[C1]], i32 789, i32 [[S2]]
+; CHECK-NEXT:    ret i32 [[S3]]
+;
+  %s1 = select i1 %c1, i32 23, i32 45
+  call void @use32(i32 %s1)
+  %s2 = select i1 %c2, i32 666, i32 %s1
+  %s3 = select i1 %c1, i32 789, i32 %s2
+  ret i32 %s3
+}
+
+define i8 @test_replace_freeze_multiuse(i1 %x, i8 %y) {
+; CHECK-LABEL: @test_replace_freeze_multiuse(
+; CHECK-NEXT:    [[EXT:%.*]] = zext i1 [[X:%.*]] to i8
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw i8 [[EXT]], [[Y:%.*]]
+; CHECK-NEXT:    [[SHL_FR:%.*]] = freeze i8 [[SHL]]
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[X]], i8 0, i8 [[SHL_FR]]
+; CHECK-NEXT:    [[ADD:%.*]] = add i8 [[SHL_FR]], [[SEL]]
+; CHECK-NEXT:    ret i8 [[ADD]]
+;
+  %ext = zext i1 %x to i8
+  %shl = shl nuw i8 %ext, %y
+  %shl.fr = freeze i8 %shl
+  %sel = select i1 %x, i8 0, i8 %shl.fr
+  %add = add i8 %shl.fr, %sel
+  ret i8 %add
+}
+
+define i8 @test_replace_freeze_oneuse(i1 %x, i8 %y) {
+; CHECK-LABEL: @test_replace_freeze_oneuse(
+; CHECK-NEXT:    [[EXT:%.*]] = zext i1 [[X:%.*]] to i8
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw i8 [[EXT]], [[Y:%.*]]
+; CHECK-NEXT:    [[SHL_FR:%.*]] = freeze i8 [[SHL]]
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[X]], i8 0, i8 [[SHL_FR]]
+; CHECK-NEXT:    ret i8 [[SEL]]
+;
+  %ext = zext i1 %x to i8
+  %shl = shl nuw i8 %ext, %y
+  %shl.fr = freeze i8 %shl
+  %sel = select i1 %x, i8 0, i8 %shl.fr
+  ret i8 %sel
+}
diff --git a/llvm/test/Transforms/PGOProfile/chr.ll b/llvm/test/Transforms/PGOProfile/chr.ll
index 0551a171091ca..38e8f8536a19c 100644
--- a/llvm/test/Transforms/PGOProfile/chr.ll
+++ b/llvm/test/Transforms/PGOProfile/chr.ll
@@ -1298,11 +1298,12 @@ define i32 @test_chr_14(ptr %i, ptr %j, i32 %sum0, i1 %pred, i32 %z) !prof !14 {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[Z_FR:%.*]] = freeze i32 [[Z:%.*]]
 ; CHECK-NEXT:    [[I0:%.*]] = load i32, ptr [[I:%.*]], align 4
-; CHECK-NEXT:    [[V1:%.*]] = icmp eq i32 [[Z_FR]], 1
-; CHECK-NEXT:    br i1 [[V1]], label [[BB1:%.*]], label [[ENTRY_SPLIT_NONCHR:%.*]], !prof [[PROF15]]
+; CHECK-NEXT:    [[V1_NOT:%.*]] = icmp eq i32 [[Z_FR]], 1
+; CHECK-NEXT:    br i1 [[V1_NOT]], label [[BB1:%.*]], label [[ENTRY_SPLIT_NONCHR:%.*]], !prof [[PROF15]]
 ; CHECK:       entry.split.nonchr:
+; CHECK-NEXT:    [[PRED_FR:%.*]] = freeze i1 [[PRED:%.*]]
 ; CHECK-NEXT:    [[V0:%.*]] = icmp eq i32 [[Z_FR]], 0
-; CHECK-NEXT:    [[V3_NONCHR:%.*]] = and i1 [[V0]], [[PRED:%.*]]
+; CHECK-NEXT:    [[V3_NONCHR:%.*]] = and i1 [[V0]], [[PRED_FR]]
 ; CHECK-NEXT:    br i1 [[V3_NONCHR]], label [[BB0_NONCHR:%.*]], label [[BB1]], !prof [[PROF16]]
 ; CHECK:       bb0.nonchr:
 ; CHECK-NEXT:    call void @foo()

llvm/test/Transforms/InstCombine/select.ll

nikic

LGTM

dtcxzyw · 2024-05-10T00:10:54Z

@AtariDreams Please don't rebase your patch unless there are some conflicts to be resolved.

At least you should tell us what you did.

…vm#91215) See the LangRef: > All uses of a value returned by the same ‘freeze’ instruction are guaranteed to always observe the same value, while different ‘freeze’ instructions may yield different values. It is incorrect to replace freezes with the simplified value. Proof: https://alive2.llvm.org/ce/z/3Dn9Cd https://alive2.llvm.org/ce/z/Qyh5h6 Fixes llvm#91178 (cherry picked from commit d085b42) Revert "[InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (llvm#91215)" This reverts commit 1c2eb18d52976fef89972e89c52d2ec5ed7e4868. [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (llvm#91215) See the LangRef: > All uses of a value returned by the same ‘freeze’ instruction are guaranteed to always observe the same value, while different ‘freeze’ instructions may yield different values. It is incorrect to replace freezes with the simplified value. Proof: https://alive2.llvm.org/ce/z/3Dn9Cd https://alive2.llvm.org/ce/z/Qyh5h6 Fixes llvm#91178 (cherry picked from commit d085b42)

tstellar · 2024-05-14T23:33:27Z

@AtariDreams (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

AtariDreams requested a review from nikic as a code owner May 8, 2024 02:23

llvmbot added PGO Profile Guided Optimizations llvm:analysis llvm:transforms labels May 8, 2024

dtcxzyw requested changes May 8, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/select.ll Outdated Show resolved Hide resolved

AtariDreams requested a review from dtcxzyw May 8, 2024 02:41

AtariDreams force-pushed the freeze branch from 1c2eb18 to 87ae76d Compare May 8, 2024 02:41

nikic added this to the LLVM 18.X Release milestone May 8, 2024

nikic approved these changes May 9, 2024

View reviewed changes

dtcxzyw mentioned this pull request May 9, 2024

[InstSimplify] Do not simplify freeze in simplifyWithOpReplaced #91215

Merged

AtariDreams force-pushed the freeze branch from 87ae76d to d3ff5d5 Compare May 9, 2024 23:36

tstellar force-pushed the freeze branch from d3ff5d5 to 0a5378e Compare May 14, 2024 21:17

tstellar force-pushed the freeze branch from 0a5378e to 494847b Compare May 14, 2024 23:32

tstellar merged commit 494847b into llvm:release/18.x May 14, 2024
7 of 8 checks passed

AtariDreams deleted the freeze branch May 14, 2024 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release/18.x: [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (#91215) #91419

release/18.x: [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (#91215) #91419

AtariDreams commented May 8, 2024

llvmbot commented May 8, 2024 •

edited

nikic left a comment

dtcxzyw commented May 10, 2024

tstellar commented May 14, 2024

release/18.x: [InstSimplify] Do not simplify freeze in simplifyWithOpReplaced (#91215) #91419

release/18.x: [InstSimplify] Do not simplify freeze in simplifyWithOpReplaced (#91215) #91419

Conversation

AtariDreams commented May 8, 2024

llvmbot commented May 8, 2024 • edited

nikic left a comment

Choose a reason for hiding this comment

dtcxzyw commented May 10, 2024

tstellar commented May 14, 2024

release/18.x: [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (#91215) #91419

release/18.x: [InstSimplify] Do not simplify freeze in `simplifyWithOpReplaced` (#91215) #91419

llvmbot commented May 8, 2024 •

edited