llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28k stars 11.56k forks source link

[X86] Non-optimal codegen for bool to float conversions with `select i1 %0, float 1.0, float 0.0` #107034

Open soooch opened 2 weeks ago

soooch commented 2 weeks ago

rust src: https://godbolt.org/z/3jjdq1ex1 c src: https://godbolt.org/z/q847cx4G3

Given the following Rust source:

#[no_mangle]
pub fn ifelse(x: bool) -> f32 {
    if x { 1. } else { 0. }
}

We end up with the following before x86 inst selection:

define noundef float @ifelse(i1 noundef zeroext %x) unnamed_addr {
start:
  %. = select i1 %x, float 1.000000e+00, float 0.000000e+00
  ret float %.
}

And eventually the following x86 codegen:

.LCPI1_0:
        .long   0x3f800000
ifelse:
        test    edi, edi
        jne     .LBB1_1
        xorps   xmm0, xmm0
        ret
.LBB1_1:
        movss   xmm0, dword ptr [rip + .LCPI1_0]
        ret

Ideally we'd probably like to see this converted to:

define noundef float @cast(i1 noundef zeroext %x) unnamed_addr {
start:
  %_0 = uitofp i1 %x to float
  ret float %_0
}

On x86, this would give us:

cast:
        cvtsi2ss        xmm0, edi
        ret

On the integer side, InstCombinePass is able to transform select i1 %x, i32 1, i32 0 to zext i1 %x to i32. That seems like a somewhat similar operation, but I'm not very familiar with LLVM.

llvmbot commented 2 weeks ago

@llvm/issue-subscribers-clang-codegen

Author: Suchir Kavi (soooch)

https://godbolt.org/z/3jjdq1ex1 Given the following Rust source: ```Rust #[no_mangle] pub fn ifelse(x: bool) -> f32 { if x { 1. } else { 0. } } ``` We end up with the following before x86 inst selection: ```llvm define noundef float @ifelse(i1 noundef zeroext %x) unnamed_addr { start: %. = select i1 %x, float 1.000000e+00, float 0.000000e+00 ret float %. } ``` And eventually the following x86 codegen: ```asm .LCPI1_0: .long 0x3f800000 ifelse: test edi, edi jne .LBB1_1 xorps xmm0, xmm0 ret .LBB1_1: movss xmm0, dword ptr [rip + .LCPI1_0] ret ``` Ideally we'd probably like to see this converted to: ```llvm define noundef float @cast(i1 noundef zeroext %x) unnamed_addr { start: %_0 = uitofp i1 %x to float ret float %_0 } ``` On x86, this would give us: ```asm cast: cvtsi2ss xmm0, edi ret ``` On the integer side, `InstCombinePass` is able to transform `select i1 %x, i32 1, i32 0` to `zext i1 %x to i32`. That seems like a somewhat similar operation, but I'm not very familiar with LLVM.
soooch commented 2 weeks ago

Not sure how to change the label, but it should probably be missed-optimization

DianQK commented 2 weeks ago

alive2: https://alive2.llvm.org/ce/z/3Y7Q-u

llvmbot commented 2 weeks ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 2 weeks ago

@llvm/issue-subscribers-good-first-issue

Author: Suchir Kavi (soooch)

rust src: https://godbolt.org/z/3jjdq1ex1 c src: https://godbolt.org/z/q847cx4G3 Given the following Rust source: ```Rust #[no_mangle] pub fn ifelse(x: bool) -> f32 { if x { 1. } else { 0. } } ``` We end up with the following before x86 inst selection: ```llvm define noundef float @ifelse(i1 noundef zeroext %x) unnamed_addr { start: %. = select i1 %x, float 1.000000e+00, float 0.000000e+00 ret float %. } ``` And eventually the following x86 codegen: ```asm .LCPI1_0: .long 0x3f800000 ifelse: test edi, edi jne .LBB1_1 xorps xmm0, xmm0 ret .LBB1_1: movss xmm0, dword ptr [rip + .LCPI1_0] ret ``` Ideally we'd probably like to see this converted to: ```llvm define noundef float @cast(i1 noundef zeroext %x) unnamed_addr { start: %_0 = uitofp i1 %x to float ret float %_0 } ``` On x86, this would give us: ```asm cast: cvtsi2ss xmm0, edi ret ``` On the integer side, `InstCombinePass` is able to transform `select i1 %x, i32 1, i32 0` to `zext i1 %x to i32`. That seems like a somewhat similar operation, but I'm not very familiar with LLVM.
soooch commented 2 weeks ago

I'm interested in trying this out if no one else is already planning on it.

Would appreciate an example of a good, similar PR if anyone has one off hand.

soooch commented 2 weeks ago

found where the "select c 1 0 -> zext c" transform is being done for ints: https://github.com/llvm/llvm-project/blob/3bc38fb27a12f785d8e78b8d00cbd277464ace92/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L3683

seems it's just inlined in the top level function. assume it's probably best to break the logic out before adding to it?

phoebewang commented 2 weeks ago

Not sure if all target can benefit from it (e.g. no native FP support). If you just want to change for X86, I think you may do it in combineSelectOfTwoConstants.

RKSimon commented 2 weeks ago

https://clang.godbolt.org/z/nP35reYMz

I'd definitely recommend this is done in the DAG and not the middle end. Best to start with x86 and we can consider generalizing it in the future.

llvmbot commented 2 weeks ago

@llvm/issue-subscribers-backend-x86

Author: Suchir Kavi (soooch)

rust src: https://godbolt.org/z/3jjdq1ex1 c src: https://godbolt.org/z/q847cx4G3 Given the following Rust source: ```Rust #[no_mangle] pub fn ifelse(x: bool) -> f32 { if x { 1. } else { 0. } } ``` We end up with the following before x86 inst selection: ```llvm define noundef float @ifelse(i1 noundef zeroext %x) unnamed_addr { start: %. = select i1 %x, float 1.000000e+00, float 0.000000e+00 ret float %. } ``` And eventually the following x86 codegen: ```asm .LCPI1_0: .long 0x3f800000 ifelse: test edi, edi jne .LBB1_1 xorps xmm0, xmm0 ret .LBB1_1: movss xmm0, dword ptr [rip + .LCPI1_0] ret ``` Ideally we'd probably like to see this converted to: ```llvm define noundef float @cast(i1 noundef zeroext %x) unnamed_addr { start: %_0 = uitofp i1 %x to float ret float %_0 } ``` On x86, this would give us: ```asm cast: cvtsi2ss xmm0, edi ret ``` On the integer side, `InstCombinePass` is able to transform `select i1 %x, i32 1, i32 0` to `zext i1 %x to i32`. That seems like a somewhat similar operation, but I'm not very familiar with LLVM.