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.

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

Jump to bottom

Invalid lowering of llvm.*.f128 intrinsics #44744

Open
legrosbuffle opened this issue Apr 2, 2020 · 1 comment · May be fixed by #76558
Open

Invalid lowering of llvm.*.f128 intrinsics #44744

legrosbuffle opened this issue Apr 2, 2020 · 1 comment · May be fixed by #76558
Labels
bugzilla Issues migrated from bugzilla llvm:codegen

Comments

@legrosbuffle
Copy link
Contributor

Bugzilla Link 45399
Version trunk
OS Linux

Extended Description

llvm.sin.f128 is incorrectly lowered to a call to sinl, which takes a f80: https://godbolt.org/z/M9fjLW

define fp128 @​bad_lowering(fp128 %a) {
  %s = call fp128 @​llvm.sin.f128(fp128 %a)
  ret fp128 %s
}

generates:

bad_lowering:
  jmp sinl

which is missing conversions from and to x86_fp80.

I expect a correct lowering to be:

define fp128 @​manual_correct_lowering(fp128 %a) {
  %t = fptrunc fp128 %a to x86_fp80
  %s = call x86_fp80 @​llvm.sin.f80(x86_fp80 %t)
  %e = fpext x86_fp80 %s to fp128
  ret fp128 %e
}

manual_correct_lowering:
  subq $24, %rsp
  callq __trunctfxf2
  fstpt (%rsp)
  callq sinl
  fstpt (%rsp)
  callq __extendxftf2
  addq $24, %rsp
  retq
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
tgross35 added a commit to tgross35/llvm-project that referenced this issue Aug 12, 2023
@tgross35
[IR] Correct lowering of f128 intrinsics
f6be88b 
Change the lowering of 128-bit floating point math function intrinsics
to use binary128 functions (`sqrtf128`) rather than `long double`
functions (`sqrtl`).

Currently intrinsic calls such as `@llvm.sqrt.f128` are lowered to
libc's `long double` functions. On platforms where `long double` is
not `fp128`, this results in incorrect math.

    define fp128 @test_sqrt(fp128 %a) {
    start:
      %0 = tail call fp128 @llvm.sqrt.f128(fp128 %a)
      ret fp128 %0
    }

    declare fp128 @llvm.sqrt.f128(fp128)

lowers to

    test_sqrt:                              # @test_sqrt
            jmp     sqrtl@PLT                       # TAILCALL

On x86 this results in the binary128 argument being treated as 80-bit
extended precision.

This has no effect on clang, which lowers builtins to the libc calls
directly without going through LLVM intrinsics.

Fixes llvm#44744
tgross35 added a commit to tgross35/llvm-project that referenced this issue Aug 14, 2023
@tgross35
[IR] Add an xpassing test for f128 intrinsic lowering
9364dee 
`f128` intrinsic functions lower to incorrect libc calls. Add a test
showing current behavior.

[IR] Correct lowering of `f128` intrinsics

Change the lowering of 128-bit floating point math function intrinsics
to use binary128 functions (`sqrtf128`) rather than `long double`
functions (`sqrtl`).

Currently intrinsic calls such as `@llvm.sqrt.f128` are lowered to
libc's `long double` functions. On platforms where `long double` is
not `fp128`, this results in incorrect math.

    define fp128 @test_sqrt(fp128 %a) {
    start:
      %0 = tail call fp128 @llvm.sqrt.f128(fp128 %a)
      ret fp128 %0
    }

    declare fp128 @llvm.sqrt.f128(fp128)

lowers to

    test_sqrt:                              # @test_sqrt
            jmp     sqrtl@PLT                       # TAILCALL

On x86 this results in the binary128 argument being treated as 80-bit
extended precision.

This has no effect on clang, which lowers builtins to the libc calls
directly without going through LLVM intrinsics.

Fixes llvm#44744

Differential Revision: https://reviews.llvm.org/D157836
@tgross35
Copy link

I started a patch for this here https://reviews.llvm.org/D157836

@tgross35 tgross35 mentioned this issue Aug 31, 2023
Merged
@tgross35 tgross35 mentioned this issue Dec 4, 2023
Closed
@tgross35 tgross35 linked a pull request Dec 29, 2023 that will close this issue
Draft
tgross35 added a commit to tgross35/llvm-project that referenced this issue Dec 30, 2023
@tgross35
[IR] Change fp128 lowering to use f128 functions by default
639f761 
Switch from emitting long double functions to using `f128`-specific functions.

Fixes llvm#44744.
tgross35 added a commit to tgross35/llvm-project that referenced this issue Jan 13, 2024
@tgross35
[IR] Change fp128 lowering to use f128 functions by default
c333dc5 
Switch from emitting long double functions to using `f128`-specific functions.

Fixes llvm#44744.
tgross35 added a commit to tgross35/llvm-project that referenced this issue Jan 20, 2024
@tgross35
[IR] Change fp128 lowering to use f128 functions by default
3739693 
Switch from emitting long double functions to using `f128`-specific functions.

Fixes llvm#44744.
tgross35 added a commit to tgross35/rust that referenced this issue Jul 30, 2024
@tgross35
Add a disclaimer about x86 f128 math functions
d64bbb1 
Due to a LLVM bug, `f128` math functions link successfully but LLVM
chooses the wrong symbols (`long double` symbols rather than those for
binary128).

Since this is a notable problem that may surprise a number of users, add
a note about it.

Link: llvm/llvm-project#44744
tgross35 added a commit to tgross35/rust that referenced this issue Aug 1, 2024
@tgross35
Add a disclaimer about x86 f128 math functions
8e2ca0c 
Due to a LLVM bug, `f128` math functions link successfully but LLVM
chooses the wrong symbols (`long double` symbols rather than those for
binary128).

Since this is a notable problem that may surprise a number of users, add
a note about it.

Link: llvm/llvm-project#44744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla llvm:codegen
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] Correct lowering of fp128 intrinsics tgross35/llvm-project
2 participants
@legrosbuffle @tgross35