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

[MIR] Consider lowering some intrinsic functions to MIR #32716

Closed
Aatch opened this issue Apr 3, 2016 · 10 comments · Fixed by #79049
Closed

[MIR] Consider lowering some intrinsic functions to MIR #32716

Aatch opened this issue Apr 3, 2016 · 10 comments · Fixed by #79049
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aatch
Copy link
Contributor

Aatch commented Apr 3, 2016

Some intrinsic functions can be effectively implemented in pure MIR code. Notably, the wrapping and checked arithmetic functions, which can turn into regular BinaryOp and CheckedBinaryOp (when implemented) rvalues. Similarly intrinsics like move_val_init can be exactly represented in the MIR.

While relatively low impact right now, due to most intrinsics being used via wrapper functions, if/when we get MIR-level inlining, this would allow those intrinsics to better participate in optimisations.

@Aatch Aatch added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Apr 4, 2016
@luqmana
Copy link
Member

luqmana commented Apr 17, 2016

I had a branch that did exactly this a while back for transmute and a few others.

https://github.com/luqmana/rust/commits/mir-intrinsics

@Aatch
Copy link
Contributor Author

Aatch commented Apr 17, 2016

@luqmana I have a branch that I was experimenting with that did the lowering while building, which I think makes more sense in some situations, though an intrinsic lowering pass in other cases may also be useful.

@aochagavia
Copy link
Contributor

I would like to work on this, but don't know where to begin. Would it make sense to rebase @luqmana's branch first?

@nagisa
Copy link
Member

nagisa commented Apr 1, 2017

Rebasing probably makes little sense. It’s a very old branch approaching the problem from a direction that would likely not work anymore, due to how much MIR has changed over time. IMO it would be easier to begin from scratch.

@aochagavia
Copy link
Contributor

aochagavia commented Apr 1, 2017

Thanks! I took a quick look at core::intrinsics and rustc_trans::intrinsic to get a bit of background information. Just to ensure I got the basics right, let me know if the following statements are correct:

  1. intrinsics are just functions that are generated directly by the compiler (instead of being compiled Rust code, like most of the standard library). For instance, there is no way mem::size_of could be implemented as a normal function, so it is generated by the compiler.
  2. Currently, intrinsics are generated by trans, which has a couple of downsides (they don't benefit from MIR optimizations, they cause code repetition if new backends are added, etc).
  3. Lots of intrinsics seem backend-specific. For instance, float arithmetic intrinsics correspond to LLVM instructions.
  4. Some intrinsics are not tied to a particular backend and can therefore be moved to MIR (mem::transmute seems like a good example, as it shouldn't generate any code at all. EDIT: I see it doesn't even exist in rustc_trans::intrinsic).

I have more questions, but I think I better make sure we are on the same line before I go on 😉

@nagisa
Copy link
Member

nagisa commented Apr 1, 2017

they don't benefit from MIR optimizations

Rather than not benefiting, they inhibit them.

Everything else sounds about right.

@aochagavia
Copy link
Contributor

Right, so now on with more questions. When you say that @luqmana's direction won't work anymore, do you mean that it would be a bad idea to write a MIR transformation that lowers intrinsics? I cannot imagine a different way of implementing this.

@nagisa
Copy link
Member

nagisa commented Apr 1, 2017

MIR transformation will still work for most of the interesting intrinsics I think; although there may be a saner approach. What I mean is that the transformation in question was written a year ago, and MIR has changed so significantly since, that rebasing it properly would be equivalent (or even harder) than redoing it from scratch.

Also: when implementing this keep in mind the eventual move to EBBs.

@aochagavia
Copy link
Contributor

Loosely based on @luqmana's code I wrote the following: https://github.com/aochagavia/rust/commit/0f9a70b3567b76b8d06ac3fd5f38426512d26128

There are a couple of FIXMEs, though. I left comments on some lines. @nagisa could you take a look at them and point me in the right direction? That would be great!

@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 24, 2017
@steveklabnik
Copy link
Member

Triage: is this bug still useful?

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 29, 2020
@bors bors closed this as completed in 361c4ea Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants