Skip to content

[InstCombine] Re-queue users of phi when nsw/nuw flags of add are inferred #113933

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 18, 2024
Merged

[InstCombine] Re-queue users of phi when nsw/nuw flags of add are inferred #113933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60e6592
[InstCombine] Add users of phi when nsw/nuw flags of add are inferred
dtcxzyw Oct 28, 2024
d2a5040
[InstCombine] Address review comments.
dtcxzyw Nov 16, 2024
890632e
[InstCombine] Use `matchSimpleRecurrence`
dtcxzyw Nov 17, 2024
faeb757
[InstCombine] Update tests. NFC.
dtcxzyw Nov 18, 2024
a8c1198
[InstCombine] Fix comments. NFC.
dtcxzyw Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1868,6 +1868,15 @@ Instruction *InstCombinerImpl::visitAdd(BinaryOperator &I) {
if (Instruction *Res = foldBinOpOfSelectAndCastOfSelectCondition(I))
return Res;

// Re-enqueue users of the induction variable of add recurrence if we infer
// new nuw/nsw flags.
if (Changed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're only interested in recurrence phis here, right? So can we check first if an add operand is a phi, and then whether it is in the user list? That way we can avoid the full users scan in most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not mentioning it earlier, but I think the cleanest way to do this is to use matchSimpleRecurrence to get the PHINode. If it's not a simple recurrence we're unlikely to get anything useful out of it anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

PHINode *PHI;
Value *Start, *Step;
if (matchSimpleRecurrence(&I, PHI, Start, Step))
Worklist.pushUsersToWorkList(*PHI);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This obviously has a positive effect, but I don't quite understand why this isn't already handled by pushUsersToWorkList in the normal InstCombine loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't quite understand why this isn't already handled by pushUsersToWorkList in the normal InstCombine loop

Take the above case as an example. If the add instruction is updated, %iv and %phi will be re-queued. But %cmp cannot be re-queued since we do not change %iv.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah duh


return Changed ? &I : nullptr;
}

Expand Down
13 changes: 3 additions & 10 deletions llvm/test/Transforms/InstCombine/2007-10-31-RangeCrash.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,18 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt < %s -S -passes=instcombine | FileCheck %s

; We do not reach a fixpoint, because we first have to infer nsw on the IV add,
; and could eliminate the icmp slt afterwards, but don't revisit it.

target datalayout = "E-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f128:64:128"

define i32 @test() "instcombine-no-verify-fixpoint" {
; CHECK-LABEL: define i32 @test(
; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT: entry:
; CHECK-NEXT: br i1 true, label [[BB_I:%.*]], label [[CALCULATECOLORSPECIFICBLACKLEVEL_EXIT:%.*]]
; CHECK: bb.i:
; CHECK-NEXT: br label [[BB51_I_I:%.*]]
; CHECK: bb27.i.i:
; CHECK-NEXT: [[TMP50_I_I:%.*]] = add nsw i32 [[X_0_I_I:%.*]], 2
; CHECK-NEXT: br label [[BB51_I_I]]
; CHECK: bb51.i.i:
; CHECK-NEXT: [[X_0_I_I]] = phi i32 [ [[TMP50_I_I]], [[BB27_I_I:%.*]] ], [ 0, [[BB_I]] ]
; CHECK-NEXT: [[TMP54_I_I:%.*]] = icmp slt i32 [[X_0_I_I]], 0
; CHECK-NEXT: br i1 [[TMP54_I_I]], label [[BB27_I_I]], label [[BB57_I_I:%.*]]
; CHECK-NEXT: br i1 false, label [[BB27_I_I:%.*]], label [[BB57_I_I:%.*]]
; CHECK: bb57.i.i:
; CHECK-NEXT: ret i32 0
; CHECK: calculateColorSpecificBlackLevel.exit:
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/Transforms/InstCombine/cast_phi.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,14 +309,14 @@ exit:
ret i64 %r
}

define i8 @trunc_in_loop_exit_block() "instcombine-no-verify-fixpoint" {
define i8 @trunc_in_loop_exit_block() {
; CHECK-LABEL: @trunc_in_loop_exit_block(
; CHECK-NEXT: entry:
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP_LATCH:%.*]] ]
; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ 1, [[ENTRY]] ], [ [[IV_NEXT]], [[LOOP_LATCH]] ]
; CHECK-NEXT: [[CMP:%.*]] = icmp ult i32 [[IV]], 100
; CHECK-NEXT: [[CMP:%.*]] = icmp samesign ult i32 [[IV]], 100
; CHECK-NEXT: br i1 [[CMP]], label [[LOOP_LATCH]], label [[EXIT:%.*]]
; CHECK: loop.latch:
; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1
Expand Down
23 changes: 6 additions & 17 deletions llvm/test/Transforms/SimpleLoopUnswitch/2007-08-01-LCSSA.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,29 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt < %s -S -passes='loop(simple-loop-unswitch),instcombine' -verify-memoryssa | FileCheck %s

; We do not reach a fixpoint, because we first have to infer nsw on the IV add,
; and could eliminate the icmp slt afterwards, but don't revisit it.

@.str9 = external constant [1 x i8]

declare i32 @strcmp(ptr, ptr)

define i32 @_ZN9Generator6strregEPKc(ptr %this, ptr %s) "instcombine-no-verify-fixpoint" {
define i32 @_ZN9Generator6strregEPKc(ptr %this, ptr %s) {
; CHECK-LABEL: define i32 @_ZN9Generator6strregEPKc(
; CHECK-SAME: ptr [[THIS:%.*]], ptr [[S:%.*]]) #[[ATTR0:[0-9]+]] {
; CHECK-SAME: ptr [[THIS:%.*]], ptr [[S:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[TMP122:%.*]] = icmp eq ptr [[S]], null
; CHECK-NEXT: br label [[BB184:%.*]]
; CHECK: bb55:
; CHECK-NEXT: ret i32 0
; CHECK: bb88:
; CHECK-NEXT: br i1 [[TMP122]], label [[BB154:%.*]], label [[BB128:%.*]]
; CHECK-NEXT: br i1 poison, label [[BB154:%.*]], label [[BB128:%.*]]
; CHECK: bb128:
; CHECK-NEXT: [[TMP138:%.*]] = call i32 @strcmp(ptr noundef nonnull dereferenceable(1) null, ptr noundef nonnull dereferenceable(1) [[S]])
; CHECK-NEXT: [[IFTMP_37_0_IN4:%.*]] = icmp eq i32 [[TMP138]], 0
; CHECK-NEXT: br i1 [[IFTMP_37_0_IN4]], label [[BB250:%.*]], label [[BB166:%.*]]
; CHECK-NEXT: br i1 poison, label [[BB250:%.*]], label [[BB166:%.*]]
; CHECK: bb154:
; CHECK-NEXT: br i1 false, label [[BB250]], label [[BB166]]
; CHECK: bb166:
; CHECK-NEXT: [[TMP175:%.*]] = add i32 [[IDX_0:%.*]], 1
; CHECK-NEXT: [[TMP183:%.*]] = add nsw i32 [[I33_0:%.*]], 1
; CHECK-NEXT: br label [[BB184]]
; CHECK: bb184:
; CHECK-NEXT: [[I33_0]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[TMP183]], [[BB166]] ]
; CHECK-NEXT: [[IDX_0]] = phi i32 [ 0, [[ENTRY]] ], [ [[TMP175]], [[BB166]] ]
; CHECK-NEXT: [[TMP49:%.*]] = icmp slt i32 [[I33_0]], 0
; CHECK-NEXT: br i1 [[TMP49]], label [[BB88:%.*]], label [[BB55:%.*]]
; CHECK-NEXT: br i1 false, label [[BB88:%.*]], label [[BB55:%.*]]
; CHECK: bb250:
; CHECK-NEXT: ret i32 [[IDX_0]]
; CHECK-NEXT: ret i32 poison
;
entry:
%s_addr.0 = select i1 false, ptr @.str9, ptr %s
Expand Down
Loading