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

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
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
`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 linked a pull request Dec 29, 2023 that will close this issue
tgross35 added a commit to tgross35/llvm-project that referenced this issue Dec 30, 2023
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
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
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
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
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
Labels
bugzilla Issues migrated from bugzilla llvm:codegen
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants