This is an archive of the discontinued LLVM Phabricator instance.

Differential D119689

[InstCombine] Add test reproducing PR53252 (NFC)
ClosedPublic

Authored by rickyz on Feb 13 2022, 10:33 PM.

Details

Reviewers
spatel
nikic
RKSimon
Commits
rG077488a6bf2e: [InstCombine] Add tests reproducing PR53252 (NFC)
Summary

Also adds some more test coverage of cases where the
canonicalizeClampLike transaction should not apply.

Diff Detail

Unit TestsFailed

TimeTest
60,340 msx64 debian > lld.COFF::incremental.test

Event Timeline

rickyz requested review of this revision.Feb 13 2022, 10:33 PM
rickyz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2022, 10:33 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B149327: Diff 408329.Feb 13 2022, 11:13 PM
spatel added a comment.Feb 15 2022, 7:32 AM

Feedback in D119690 is that we should have a few more tests to cover other predicate combinations.

rickyz updated this revision to Diff 409086.Feb 15 2022, 4:05 PM

Add test coverage of the other unsigned compare cases.

rickyz mentioned this in D119690: [InstCombine] Update predicate when canonicalizing comparisons in canonicalizeClampLike..Feb 15 2022, 4:22 PM
rickyz added inline comments.Feb 15 2022, 4:25 PM
llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-negative-and-positive-thresholds.ll
201

I omitted the constant offset on x (add i32 %x, C0) in these tests because other transformations that these tests aren't trying to exercise would rewrite them. Happy to add them if preferred - it'd just mean that changes to those other transformations would cause churn in these tests.

Harbormaster completed remote builds in B149854: Diff 409086.Feb 15 2022, 6:09 PM
rickyz updated this revision to Diff 409114.Feb 15 2022, 6:14 PM
rickyz retitled this revision from [InstCombine] Add test reproducing PR53252 (NFC) to [InstCombine] Add tests reproducing PR53252 (NFC).
rickyz edited the summary of this revision. (Show Details)

Update description.

Harbormaster completed remote builds in B149872: Diff 409114.Feb 15 2022, 6:56 PM
rickyz updated this revision to Diff 409843.Feb 17 2022, 7:33 PM

Remove non-strict comparison tests.

These do not add any real coverage, as any interesting comparisons
against constants will have been canonicalized to use strict predicates
beforehand.

Harbormaster completed remote builds in B150371: Diff 409843.Feb 17 2022, 8:16 PM
rickyz added a child revision: D119690: [InstCombine] Update predicate when canonicalizing comparisons in canonicalizeClampLike..Apr 26 2022, 12:05 AM
RKSimon accepted this revision.Apr 26 2022, 1:15 AM
RKSimon added a subscriber: RKSimon.

LGTM with one minor

llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-negative-and-positive-thresholds.ll
190

(trivial) Move the FIXME above the define

This revision is now accepted and ready to land.Apr 26 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 1:15 AM
rickyz updated this revision to Diff 425148.Apr 26 2022, 1:45 AM

Add negative tests exercising Pred0 = {UGE, ULE}.

rickyz updated this revision to Diff 425150.Apr 26 2022, 1:45 AM

Rebase.

rickyz updated this revision to Diff 425159.EditedApr 26 2022, 2:25 AM
rickyz marked an inline comment as done.

Update description to clarify that this change only adds a single test triggering PR53252.

rickyz updated this revision to Diff 425161.Apr 26 2022, 2:27 AM
rickyz retitled this revision from [InstCombine] Add tests reproducing PR53252 (NFC) to [InstCombine] Add test reproducing PR53252 (NFC).

Update description for real.

Forgot to specify --verbatim, sorry for the noise!

Harbormaster completed remote builds in B161352: Diff 425161.Apr 26 2022, 5:12 AM
This revision was landed with ongoing or failed builds.Apr 26 2022, 2:24 PM
Closed by commit rG077488a6bf2e: [InstCombine] Add tests reproducing PR53252 (NFC) (authored by rickyz, committed by spatel). · Explain Why
This revision was automatically updated to reflect the committed changes.
spatel added a commit: rG077488a6bf2e: [InstCombine] Add tests reproducing PR53252 (NFC).

Revision Contents

Diff 409086

llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-negative-and-positive-thresholds.ll

Show First 20 Lines • Show All 125 Lines • ▼ Show 20 Lines
; ;
%t0 = icmp sgt i32 %x, 127 %t0 = icmp sgt i32 %x, 127
%t1 = select i1 %t0, i32 %replacement_high, i32 %replacement_low %t1 = select i1 %t0, i32 %replacement_high, i32 %replacement_low
%t2 = add i32 %x, 16 %t2 = add i32 %x, 16
%t3 = icmp ugt i32 %t2, 143 %t3 = icmp ugt i32 %t2, 143
%r = select i1 %t3, i32 %t1, i32 %x %r = select i1 %t3, i32 %t1, i32 %x
ret i32 %r ret i32 %r
} }
define i32 @t7_ugt_sgt_neg1(i32 %x, i32 %replacement_low, i32 %replacement_high) { define i32 @t7_ugt_sgt_neg1(i32 %x, i32 %replacement_low, i32 %replacement_high) {
; CHECK-LABEL: @t7_ugt_sgt_neg1( ; CHECK-LABEL: @t7_ugt_sgt_neg1(
; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[X:%.*]], -16 ; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[X:%.*]], -16
; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[X]], 127 ; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[X]], 127
; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP1]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[X]] ; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP1]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[X]]
; CHECK-NEXT: [[R:%.*]] = select i1 [[TMP2]], i32 [[REPLACEMENT_HIGH:%.*]], i32 [[TMP3]] ; CHECK-NEXT: [[R:%.*]] = select i1 [[TMP2]], i32 [[REPLACEMENT_HIGH:%.*]], i32 [[TMP3]]
; CHECK-NEXT: ret i32 [[R]] ; CHECK-NEXT: ret i32 [[R]]
; ;
Show All 20 Lines
; ;
%t0 = icmp slt i32 %x, 129 %t0 = icmp slt i32 %x, 129
%t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high %t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
%t2 = add i32 %x, 16 %t2 = add i32 %x, 16
%t3 = icmp ult i32 %t2, 144 %t3 = icmp ult i32 %t2, 144
%r = select i1 %t3, i32 %x, i32 %t1 %r = select i1 %t3, i32 %x, i32 %t1
ret i32 %r ret i32 %r
} }
define i32 @n9_ult_slt_neg17(i32 %x, i32 %replacement_low, i32 %replacement_high) { define i32 @n9_ult_slt_neg17(i32 %x, i32 %replacement_low, i32 %replacement_high) {
; CHECK-LABEL: @n9_ult_slt_neg17( ; CHECK-LABEL: @n9_ult_slt_neg17(
; CHECK-NEXT: [[T0:%.*]] = icmp slt i32 [[X:%.*]], -17 ; CHECK-NEXT: [[T0:%.*]] = icmp slt i32 [[X:%.*]], -17
; CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[REPLACEMENT_HIGH:%.*]] ; CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[REPLACEMENT_HIGH:%.*]]
; CHECK-NEXT: [[T2:%.*]] = add i32 [[X]], 16 ; CHECK-NEXT: [[T2:%.*]] = add i32 [[X]], 16
; CHECK-NEXT: [[T3:%.*]] = icmp ult i32 [[T2]], 144 ; CHECK-NEXT: [[T3:%.*]] = icmp ult i32 [[T2]], 144
; CHECK-NEXT: [[R:%.*]] = select i1 [[T3]], i32 [[X]], i32 [[T1]] ; CHECK-NEXT: [[R:%.*]] = select i1 [[T3]], i32 [[X]], i32 [[T1]]
; CHECK-NEXT: ret i32 [[R]] ; CHECK-NEXT: ret i32 [[R]]
; ;
%t0 = icmp slt i32 %x, -17 %t0 = icmp slt i32 %x, -17
%t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high %t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
%t2 = add i32 %x, 16 %t2 = add i32 %x, 16
%t3 = icmp ult i32 %t2, 144 %t3 = icmp ult i32 %t2, 144
%r = select i1 %t3, i32 %x, i32 %t1 %r = select i1 %t3, i32 %x, i32 %t1
ret i32 %r ret i32 %r
} }
define i32 @n10_ugt_slt(i32 %x, i32 %replacement_low, i32 %replacement_high) {
; FIXME: This is incorrect, see PR53252.
RKSimonUnsubmitted
Done
ReplyInline Actions

(trivial) Move the FIXME above the define

RKSimon: (trivial) Move the FIXME above the define
; CHECK-LABEL: @n10_ugt_slt(
; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[X:%.*]], 0
; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[X]], 128
; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP1]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[X]]
; CHECK-NEXT: [[R:%.*]] = select i1 [[TMP2]], i32 [[REPLACEMENT_HIGH:%.*]], i32 [[TMP3]]
; CHECK-NEXT: ret i32 [[R]]
;
%t0 = icmp slt i32 %x, 0
%t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
%t2 = icmp ugt i32 %x, 128
%r = select i1 %t2, i32 %x, i32 %t1
rickyzAuthorUnsubmitted
Done
ReplyInline Actions

I omitted the constant offset on x (add i32 %x, C0) in these tests because other transformations that these tests aren't trying to exercise would rewrite them. Happy to add them if preferred - it'd just mean that changes to those other transformations would cause churn in these tests.

rickyz: I omitted the constant offset on x (add i32 %x, C0) in these tests because other…
ret i32 %r
}
define i32 @n11_uge_slt(i32 %x, i32 %replacement_low, i32 %replacement_high) {
; FIXME: This is incorrect, see PR53252.
; CHECK-LABEL: @n11_uge_slt(
; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[X:%.*]], 0
; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[X]], 127
; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP1]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[X]]
; CHECK-NEXT: [[R:%.*]] = select i1 [[TMP2]], i32 [[REPLACEMENT_HIGH:%.*]], i32 [[TMP3]]
; CHECK-NEXT: ret i32 [[R]]
;
%t0 = icmp slt i32 %x, 0
%t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
%t2 = icmp uge i32 %x, 128
%r = select i1 %t2, i32 %x, i32 %t1
ret i32 %r
}
define i32 @n12_ule_slt(i32 %x, i32 %replacement_low, i32 %replacement_high) {
; CHECK-LABEL: @n12_ule_slt(
; CHECK-NEXT: [[T0:%.*]] = icmp slt i32 [[X:%.*]], 128
; CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i32 [[REPLACEMENT_LOW:%.*]], i32 [[REPLACEMENT_HIGH:%.*]]
; CHECK-NEXT: [[T2:%.*]] = icmp ult i32 [[X]], 2
; CHECK-NEXT: [[R:%.*]] = select i1 [[T2]], i32 [[X]], i32 [[T1]]
; CHECK-NEXT: ret i32 [[R]]
;
%t0 = icmp slt i32 %x, 128
%t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
%t2 = icmp ule i32 %x, 1
%r = select i1 %t2, i32 %x, i32 %t1
ret i32 %r
}
;------------------------------------------------------------------------------- ;-------------------------------------------------------------------------------
declare void @use32(i32) declare void @use32(i32)
declare void @use1(i1) declare void @use1(i1)
; One-use restrictions: here the entire pattern needs to be one-use. ; One-use restrictions: here the entire pattern needs to be one-use.
; FIXME: if %t0 could be reused then it's less restrictive. ; FIXME: if %t0 could be reused then it's less restrictive.
▲ Show 20 Lines • Show All 285 Lines • Show Last 20 Lines