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

Missed opportunity to simplify bitselect pattern #50160

Closed
RKSimon opened this issue Jun 23, 2021 · 4 comments
Closed

Missed opportunity to simplify bitselect pattern #50160

RKSimon opened this issue Jun 23, 2021 · 4 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@RKSimon
Copy link
Collaborator

RKSimon commented Jun 23, 2021

Bugzilla Link 50816
Resolution FIXED
Resolved on Jul 13, 2021 06:55
Version trunk
OS Windows NT
CC @LebedevRI,@rotateright
Fixed by commit(s) a488c78 b2f6cf1

Extended Description

Pulled from bullet source code:

unsigned btSelect(unsigned condition, unsigned valueIfConditionNonZero, unsigned valueIfConditionZero)
{
// Set testNz to 0xFFFFFFFF if condition is nonzero, 0x00000000 if condition is zero
// Rely on positive value or'ed with its negative having sign bit on
// and zero value or'ed with its negative (which is still zero) having sign bit off
// Use arithmetic shift right, shifting the sign bit through all 32 bits
unsigned testNz = (unsigned)(((int)condition | -(int)condition) >> 31);
unsigned testEqz = ~testNz;
return ((valueIfConditionNonZero & testNz) | (valueIfConditionZero & testEqz));
}

https://c.godbolt.org/z/rMf6Ta613

The following code doesn't get simplified:

define i8 @​src(i8 %a0, i8 %a1, i8 %a2) {
%add = add i8 %a0, -1
%xor = xor i8 %a0, -1
%and = and i8 %add, %xor
%cmp = icmp sgt i8 %and, -1
%sel = select i1 %cmp, i8 %a1, i8 %a2
ret i8 %sel
}

define i8 @​tgt(i8 %a0, i8 %a1, i8 %a2) {
%cmp = icmp ne i8 %a0, 0
%sel = select i1 %cmp, i8 %a1, i8 %a2
ret i8 %sel
}

Transformation seems to be correct!

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jul 9, 2021

We don't get the simpler variant either:

define i32 @​src(i32 %0) {
%1:
%2 = sub nsw i32 0, %0
%3 = or i32 %2, %0
%4 = ashr i32 %3, 31
ret i32 %4
}
=>
define i32 @​tgt(i32 %0) {
%1:
%2 = icmp ne i32 %0, 0
%3 = sext i1 %2 to i32
ret i32 %3
}
Transformation seems to be correct!

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jul 10, 2021

Candidate Patch: https://reviews.llvm.org/D105764

@rotateright
Copy link
Contributor

Candidate Patch: https://reviews.llvm.org/D105764

I didn't check if that patch would solve the motivating source example, but I think we need the icmp fold(s) anyway, so:
https://reviews.llvm.org/rGa488c7879e68

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jul 13, 2021

Candidate Patch: https://reviews.llvm.org/D105764

Committed at rGb2f6cf14798a

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

2 participants