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

Use minnum/maxnum LLVM instrinsics for fmin/fmax #18384

Closed
huonw opened this issue Oct 28, 2014 · 7 comments · Fixed by #61408
Closed

Use minnum/maxnum LLVM instrinsics for fmin/fmax #18384

huonw opened this issue Oct 28, 2014 · 7 comments · Fixed by #61408
Assignees
@varkor
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@huonw
Copy link
Member

huonw commented Oct 28, 2014

http://reviews.llvm.org/rL220341

Requires an LLVM upgrade, but not worth doing one just for this purpose.
@huonw huonw added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. A-codegen Area: Code generation labels Oct 28, 2014
@emberian emberian self-assigned this Mar 25, 2015
@emberian emberian removed their assignment Jan 5, 2016
@huonw
Copy link
Member Author

huonw commented Jan 5, 2016

(Many LLVM upgrades have happened since then.)

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 22, 2017
varkor added a commit to varkor/rust that referenced this issue Dec 21, 2017
@varkor
Add LLVM minnum/maxnum intrinsics
de46292 
LLVM’s intrinsics `minnum` and `maxnum` are now used for `min` and
`max` on `f32` and `f64`. This resolves rust-lang#18384.
@varkor varkor mentioned this issue Dec 21, 2017
Closed
kennytm added a commit to kennytm/rust that referenced this issue Dec 23, 2017
@kennytm
Rollup merge of rust-lang#46926 - varkor:contrib-9, r=joshtriplett
5d9fe6f 
Add LLVM `minnum`/`maxnum` intrinsics

LLVM’s intrinsics `minnum` and `maxnum` are now used for `min` and
`max` on `f32` and `f64`. This resolves rust-lang#18384.
@varkor
Copy link
Member

varkor commented Jan 4, 2018

Update as of last 2017: there are still issues with optimisation of minnum/maxnum when used for clamping on x86, which seems to be the only blocker. There's an implementation here that could be merged when LLVM figures out how to optimise that case.

@nox
Copy link
Contributor

nox commented Apr 2, 2018

clamp01_llvm is still worse than clamp01_rust. Is there any upstream issue on LLVM that we could refer to here?

Cc @rust-lang/wg-codegen

@nikic
Copy link
Contributor

nikic commented May 25, 2019

The X86 codegen issue should be fixed by llvm/llvm-project@d87eced.

@varkor varkor mentioned this issue May 30, 2019
Merged
@varkor varkor self-assigned this May 30, 2019
@varkor varkor mentioned this issue May 31, 2019
Open
@varkor
Copy link
Member

varkor commented May 31, 2019 •
edited
Loading

I'm not sure if there's a problem now in that we can't use intrinsics from libcore, so we'd have to move min/max to libstd: #50145.

@varkor varkor mentioned this issue May 31, 2019
Merged
@nikic
Copy link
Contributor

nikic commented May 31, 2019

Relevant libcalls for minnum/maxnum: https://github.com/llvm-mirror/llvm/blob/29f026d977e01fc255284a0bb90afda1435b0e6a/include/llvm/IR/RuntimeLibcalls.def#L247-L256

@varkor varkor mentioned this issue May 31, 2019
Merged
bors added a commit that referenced this issue Jun 2, 2019
@bors
Auto merge of #61384 - varkor:fmin-fmax-llvm-opt, r=alexcrichton
538e17a 
Update LLVM to include fmin/fmax optimisations

This will enable us to test if the optimisation issues mentioned in #18384 really are fixed. Unfortunately, using the intrinsics immediately is problematic due to the libcore/libstd split (see #50145).
@varkor
Copy link
Member

varkor commented Jun 5, 2019

As expected, LLVM is now properly optimising the clamp pattern.

bors added a commit that referenced this issue Jun 7, 2019
@bors
Auto merge of #61408 - varkor:fmin-fmax-llvm-intrinsics, r=alexcrichton
c5295ac 
Use LLVM intrinsics for floating-point min/max

Resurrection of #46926, now that the optimisation issues are fixed. I've confirmed locally that #61384 solves the issues.

I'm not sure if we're allowed to move the `min`/`max` methods from libcore to libstd: I can't quite tell what the status is from #50145. However, this is necessary to use the intrinsics.

Fixes #18384.

r? @SimonSapin
cc @rkruppe @nikic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@varkor varkor

Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Milestone
No milestone
6 participants
@nox @nikic @emberian @huonw @varkor @Mark-Simulacrum