Skip to content

Make sin, cos, exp, exp2, log, log2, log10 generic - #160989

Open
N1ark wants to merge 3 commits into
rust-lang:mainfrom
N1ark:08-12-generic-float-intrinsincs
Open

Make sin, cos, exp, exp2, log, log2, log10 generic#160989
N1ark wants to merge 3 commits into
rust-lang:mainfrom
N1ark:08-12-generic-float-intrinsincs

Conversation

@N1ark

@N1ark N1ark commented Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Rebased and smaller version of #153934

Following fabs, make the sin, cos, exp, exp2, log, log2 and log10 intrinsics generic over the float type, rather than having four variants per float type.

The first two commits are purely stylistic:

  • reorganised Cranelift code to make following changes simpler
  • moved a misplaced comment in compiler/rustc_codegen_llvm/src/intrinsic.rs that caused x fmt to give up

The last commit actually makes them generic! Most code is a bit simpler, and this will also hopefully simplify adding support for these intrinsics for the future bf16 type :)

Unfortunately both GCC and Cranelift backend changes are a bit churny. Their code is a bit, opaque, to put it kindly, and I didn't want to refactor those here.

r? @folkertdev
cc @RalfJung

Disclosure: I used an LLM to figure out the macro code.

@rustbot

rustbot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Aug 12, 2026
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 12, 2026
@folkertdev

folkertdev commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

I suspect it would be better to submit 295e7a3 to the cg_clif repo for better CI coverage.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@N1ark
N1ark force-pushed the 08-12-generic-float-intrinsincs branch from d98ca71 to b3a59f6 Compare August 12, 2026 14:55
@rustbot

rustbot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Aug 12, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Aug 12, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
CValue::by_val(codegen_f16_f128::fma_f16(fx, args[0], args[1], args[2]), layout)
}
let val = match intrinsic {
sym::fmaf16 | sym::fmuladdf16 => codegen_f16_f128::fma_f16(fx, args[0], args[1], args[2]),

@bjorn3 bjorn3 Aug 12, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflicts with rust-lang/rustc_codegen_cranelift#1675. I'm planning to do a sync soon.

View changes since the review

@N1ark N1ark Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh then we can hold off from merging this until you've synced, i dont mind

@folkertdev folkertdev Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rustbot author

@bjorn3 bjorn3 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #161232 for the sync.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2026
RalfJung
RalfJung reviewed Aug 16, 2026
View reviewed changes

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM codegen changes LGTM.
Miri changes are missing, as CI is pointing out. :)
Personally I am fine with these macros, but it's clearly a tradeoff.

View changes since this review

Comment thread library/core/src/intrinsics/macros.rs
Comment thread tests/ui/resolve/bad-env-capture.stderr
|
help: consider importing this function
|
LL + use std::intrinsics::log;

@RalfJung RalfJung Aug 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad side-effect. Do we have an open issue to track the problem that apparently we recommend importing unstable functions?

@N1ark N1ark Aug 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that i could fine, but not sure. agreed this seems pretty bad though, especially for a name as common as log

@RalfJung RalfJung Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, something odd is happening -- I don't get similar suggestions for existing intrinsics
Playground

@RalfJung

RalfJung commented Aug 16, 2026

Copy link
Copy Markdown
Member

moved a misplaced comment in compiler/rustc_codegen_llvm/src/intrinsic.rs that caused x fmt to give up

The comment was not misplaced, it was placed exactly where it should be. rustfmt just doesn't like comments in some places so we have to move the comment into a suboptimal place. :(

@N1ark

N1ark commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

moved a misplaced comment in compiler/rustc_codegen_llvm/src/intrinsic.rs that caused x fmt to give up

The comment was not misplaced, it was placed exactly where it should be. rustfmt just doesn't like comments in some places so we have to move the comment into a suboptimal place. :(

yes sorry, i meant misplaced from rustfmt's perspective ^^ i agree it's not ideal there, it just seemed clearer than inside the match arm:/

@N1ark
N1ark force-pushed the 08-12-generic-float-intrinsincs branch from b3a59f6 to 88ea1bd Compare August 17, 2026 14:18
@N1ark
N1ark force-pushed the 08-12-generic-float-intrinsincs branch from 88ea1bd to 01eda22 Compare August 17, 2026 14:22
@N1ark N1ark changed the title Make exp, exp2, log, log2, log10 generic Make sin, cos, exp, exp2, log, log2, log10 generic Aug 17, 2026
@N1ark

N1ark commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

fyi i snuck in sin and cos into the batch because they were the same shape and fit the change really well ^^

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

Reviewers

@RalfJung RalfJung RalfJung left review comments
@folkertdev folkertdev folkertdev left review comments
@bjorn3 bjorn3 bjorn3 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@folkertdev folkertdev

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@N1ark @rustbot @folkertdev @rust-log-analyzer @RalfJung @bjorn3