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

[x86] x >/>=/</<= 0 produce very different assembly #59668

Open
scottmcm opened this issue Dec 23, 2022 · 3 comments
Open

[x86] x >/>=/</<= 0 produce very different assembly #59668

scottmcm opened this issue Dec 23, 2022 · 3 comments

Comments

@scottmcm
Copy link

Compiling a bunch of icmps against zero

define noundef zeroext i1 @check_slt(i8 %0) unnamed_addr #0 {
start:
  %r = icmp slt i8 %0, 0
  ret i1 %r
}
define noundef zeroext i1 @check_sle(i8 %0) unnamed_addr #0 {
start:
  %r = icmp sle i8 %0, 0
  ret i1 %r
}
define noundef zeroext i1 @check_sgt(i8 %0) unnamed_addr #0 {
start:
  %r = icmp sgt i8 %0, 0
  ret i1 %r
}
define noundef zeroext i1 @check_sge(i8 %0) unnamed_addr #0 {
start:
  %r = icmp sge i8 %0, 0
  ret i1 %r
}

Gives two rather different kinds of checks https://llvm.godbolt.org/z/4sh47s1bn

check_slt:                              # @check_slt
        mov     eax, edi
        shr     al, 7
        ret
check_sle:                              # @check_sle
        test    dil, dil
        setle   al
        ret
check_sgt:                              # @check_sgt
        test    dil, dil
        setg    al
        ret
check_sge:                              # @check_sge
        mov     eax, edi
        not     al
        shr     al, 7
        ret

My instinct is that check_sge should just be test+setge, more like how check_sgt is, rather than needing a shift-by-immediate.

But I'm also really not familiar with the nuances of codegen. I suppose it's also possible that flags are so bad that not using them is better. If that's the case, though, should the others be changed too? Maybe check_sgt could be done with neg and shifting, for example.

(Or perhaps how it is currently is is already best, and I should just go read more architecture manuals to learn what matters.)


Inspired by a conversation about the Rust's Ordering::is_lt & friends, and the different ways those could be written https://rust.godbolt.org/z/fq96Yezq7.

@llvmbot
Copy link
Member

llvmbot commented Dec 23, 2022

@llvm/issue-subscribers-backend-x86

@RKSimon
Copy link
Collaborator

RKSimon commented Jan 4, 2023

Interestingly this appears to only be an issue for i8

@scottmcm
Copy link
Author

Hmm, for i16, it looks like slt 0 becomes shr 15, but the others are setCC (https://llvm.godbolt.org/z/ThfGbj39v). Maybe that's fine, though? (My μarchitecture knowledge is not nearly good enough to know for sure.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants