Skip to content

[InstCombine] Fold select Cond, not X, X into Cond ^ X #90089

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

Closed
wants to merge 4 commits into from
Closed

[InstCombine] Fold select Cond, not X, X into Cond ^ X #90089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6dffd8b
[InstCombine] Add pre-commit tests. NFC.
dtcxzyw Apr 25, 2024
ac7ded4
[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X`
dtcxzyw Apr 25, 2024
d1e3e81
[InstCombine] Add a header comment for the fold. NFC.
dtcxzyw May 13, 2024
1731798
[InstCombine] Add some comments.
dtcxzyw May 17, 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
8 changes: 8 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3985,5 +3985,13 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
}
}

// select Cond, !X, X -> xor Cond, X
// Note: We don't fold select Cond, Y, X -> X (iff X->Y & !X->!Y) here as
// it indicates that these two patterns should be canonicalized.
if (CondVal->getType() == SI.getType() && impliesPoison(FalseVal, TrueVal) &&
isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == false &&
isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it cover the motivating cases to check for icmp with same operands and inverted predicate?

This is a pretty weird check and I'm not sure if doing the poison check in one direction is sufficient here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I guess it's fine as FalseVal is the value we're replacing with.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still like an answer to this question. This is a very roundabout way to check whether the conditions are inverses of each other, and it does have some overhead (http://llvm-compile-time-tracker.com/compare.php?from=76a3be7c766bd55221c3d0d0a74c42f82c5d76ed&to=1731798e2ef457fb5cabd29ae054c700b8b13c14&stat=instructions%3Au). Not enough to make me overly concerned, about enough to contribute to the death by a thousand cuts :)

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 can simplify the implementation to just handle select Cond, icmp X, C1, icmp X, C2, which covers all the motivating cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt the check be (isImpliedCondition(..., true) ^ isImpliedCondition(..., false))?

Copy link
Member Author

Choose a reason for hiding this comment

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

When FVal implies TVal and !FVal -> !TVal, TVal must be the same as FVal. If not, we need a canonicalization for this pattern.

BTW, I tried isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == true && isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == false before submitting this patch. But it didn't improve the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment that the inverse case doesn't naturally occur in code to avoid people re-testing / re-submitting.

return BinaryOperator::CreateXor(CondVal, FalseVal);

return nullptr;
}
74 changes: 74 additions & 0 deletions llvm/test/Transforms/InstCombine/select-cmp.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,4 +345,78 @@ define i1 @icmp_no_common(i1 %c, i8 %x, i8 %y, i8 %z) {
ret i1 %r
}

define i1 @test_select_inverse1(i64 %x, i1 %y) {
; CHECK-LABEL: @test_select_inverse1(
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i64 [[X:%.*]], 0
; CHECK-NEXT: [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
; CHECK-NEXT: ret i1 [[SEL]]
;
%cmp1 = icmp ne i64 %x, 0
%cmp2 = icmp eq i64 %x, 0
%sel = select i1 %y, i1 %cmp1, i1 %cmp2
ret i1 %sel
}

define i1 @test_select_inverse2(i64 %x, i1 %y) {
; CHECK-LABEL: @test_select_inverse2(
; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i64 [[X:%.*]], 0
; CHECK-NEXT: [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
; CHECK-NEXT: ret i1 [[SEL]]
;
%cmp1 = icmp sgt i64 %x, -1
%cmp2 = icmp slt i64 %x, 0
%sel = select i1 %y, i1 %cmp1, i1 %cmp2
ret i1 %sel
}

define i1 @test_select_inverse3(ptr %x, i1 %y) {
; CHECK-LABEL: @test_select_inverse3(
; CHECK-NEXT: [[CMP2:%.*]] = icmp ne ptr [[X:%.*]], null
; CHECK-NEXT: [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
; CHECK-NEXT: ret i1 [[SEL]]
;
%cmp1 = icmp eq ptr %x, null
%cmp2 = icmp ne ptr %x, null
%sel = select i1 %y, i1 %cmp1, i1 %cmp2
ret i1 %sel
}

define i1 @test_select_inverse_fail(i64 %x, i1 %y) {
; CHECK-LABEL: @test_select_inverse_fail(
; CHECK-NEXT: [[CMP1:%.*]] = icmp sgt i64 [[X:%.*]], 0
; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i64 [[X]], 0
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[Y:%.*]], i1 [[CMP1]], i1 [[CMP2]]
; CHECK-NEXT: ret i1 [[SEL]]
;
%cmp1 = icmp sgt i64 %x, 0
%cmp2 = icmp slt i64 %x, 0
%sel = select i1 %y, i1 %cmp1, i1 %cmp2
ret i1 %sel
}

define <2 x i1> @test_select_inverse_vec(<2 x i64> %x, <2 x i1> %y) {
; CHECK-LABEL: @test_select_inverse_vec(
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq <2 x i64> [[X:%.*]], zeroinitializer
; CHECK-NEXT: [[SEL:%.*]] = xor <2 x i1> [[CMP2]], [[Y:%.*]]
; CHECK-NEXT: ret <2 x i1> [[SEL]]
;
%cmp1 = icmp ne <2 x i64> %x, zeroinitializer
%cmp2 = icmp eq <2 x i64> %x, zeroinitializer
%sel = select <2 x i1> %y, <2 x i1> %cmp1, <2 x i1> %cmp2
ret <2 x i1> %sel
}

define <2 x i1> @test_select_inverse_vec_fail(<2 x i64> %x, i1 %y) {
; CHECK-LABEL: @test_select_inverse_vec_fail(
; CHECK-NEXT: [[CMP1:%.*]] = icmp ne <2 x i64> [[X:%.*]], zeroinitializer
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq <2 x i64> [[X]], zeroinitializer
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[Y:%.*]], <2 x i1> [[CMP1]], <2 x i1> [[CMP2]]
; CHECK-NEXT: ret <2 x i1> [[SEL]]
;
%cmp1 = icmp ne <2 x i64> %x, zeroinitializer
%cmp2 = icmp eq <2 x i64> %x, zeroinitializer
%sel = select i1 %y, <2 x i1> %cmp1, <2 x i1> %cmp2
ret <2 x i1> %sel
}

declare void @use(i1)
Loading