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

Rewrite platform intrinsic generation as a build script #41885

Closed
alexcrichton opened this issue May 10, 2017 · 16 comments
Closed

Rewrite platform intrinsic generation as a build script #41885

alexcrichton opened this issue May 10, 2017 · 16 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Right now the compiler has a rustc_platform_intrinsics crate which is basically a database of all platform intrinsics with their inputs/outputs and LLVM names, etc. This is the information the compiler uses to typecheck all intrinsics and also generate code for them with LLVM. An example generated file looks like aarch64.rs.

These files are all generated by a generator.py script in the src/etc dir. This generator slurps up a ton of JSON definitions, for example all the files in src/etc/platform-intrinsics/x86 and then generates these Rust files.

Now that we're using Cargo as a build system we can avoid checking in these generated source files. Instead we can translate generator.py into Rust as a build script and use build-time code generation to generate all of these files.

This isn't necessarily an easy task because generator.py is not exactly a trivial script, but I'm more than willing to help out if anyone's got any questions!
@tommyip
Copy link
Contributor

tommyip commented May 10, 2017

I would like to work on this issue!

@alexcrichton
Copy link
Member Author

@tommyip oh I believe @F001 in also expressed interest earlier in #41568 for this issue. Just in that y'all may want to coordinate to make sure no toes are stepped on!

@alexcrichton
Copy link
Member Author

(not sure why that issue link didn't show up...), also sorry, should have mentioned that in the issue earlier!

@tommyip
Copy link
Contributor

tommyip commented May 10, 2017 •
edited
Loading

ok no problem, I guess I should leave this to @F001 . @alexcrichton Are there any other scripts that need to be rewritten?

@F001
Copy link
Contributor

F001 commented May 10, 2017

Thanks. I will work on this.

@Mark-Simulacrum Mark-Simulacrum added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. labels Jul 27, 2017
@theduke
Copy link
Contributor

theduke commented Sep 1, 2017

@F001 are you still working on this?

@F001
Copy link
Contributor

F001 commented Sep 1, 2017

@theduke I'm sorry, I have not wok on this for more than two months. And I guess I won't have enough time for the comming 2 months. I have updated my code on https://github.com/F001/librustc_platform_intrinsics, in a very early stage.

If anyone want to take this, that's OK. You can start from scratch or base on my project, it's your choice. I will probably come back to this issue at least 2 months later, if no one finish this task at that time.

@Mark-Simulacrum Mark-Simulacrum added this to the impl period milestone Sep 15, 2017
@aturon aturon removed this from the impl period milestone Sep 15, 2017
@laumann
Copy link
Contributor

laumann commented Sep 26, 2017

I see this was taken out of "impl period", but it is a desirable thing to have implemented at the moment? It looks interesting, and I think I have time to look into it, so I'd be happy to give it a go (unless @tommyip wants a go first 😄).

I also have some questions: the generator.py script takes some command-line parameters, and as this is not possible in build scripts, how should the generated code be built correctly? I'm guessing the parameters should be passed through env variables. Also, how should the code then be included? Is that any different than how it is now? (Now that I'm writing it, I guess I'm wondering where the code generation is triggered from and where the generated code is included.)

@tommyip
Copy link
Contributor

tommyip commented Sep 26, 2017

@laumann go for it! I am kinda busy at the moment.

@alexcrichton
Copy link
Member Author

@laumann oh awesome! We may not want to do a straight translation of generator.py 1:1 in the sense that there may be no need to define JSON when we could just have plain old Rust or a macro of some form. Additionally it's ok to avoid command line options and just always build all intrinsics unconditionally most likely?

@laumann
Copy link
Contributor

laumann commented Sep 26, 2017

@tommyip Cool 👍

@alexcrichton I think I'm wondering where in the build process this should be hooked in. Is the idea that the librustc_platform_intrinsics crate should just generate the right files when it's compiled? At this point the target architecture should be available (the TARGET env variable), so it should be possible to generate just the intrinsics needed, but just generate them all should work too.

Are you thinking of something specific that'd be easier with a macro rather than the JSON?

@alexcrichton
Copy link
Member Author

@laumann ah yeah as a build script this'll just get naturally run and compiled whenever the crate's built. I don't think you'll need to take TARGET into account though as this includes the intrinsic definitions for all platforms all the time.

Oh whatever's easiest should be fine, I'd just imagine that long-term JSON isn't really necessary, it was probably just easy for Python.

@laumann
Copy link
Contributor

laumann commented Oct 3, 2017

@alexcrichton So I've been working on this here (btw, so sorry I never got to say hi at RöstiFest!). I forked @F001's project as a starting point. Status is that it seems mostly complete, except for values that need to be plugged into the generated match arms.

@F001 I wanted to ask why the width is an i32 and not a u32? Can it ever be negative?

@alexcrichton
Copy link
Member Author

Gah sorry I missed you @laumann! But nice job on the work here so far!

@laumann
Copy link
Contributor

laumann commented Oct 4, 2017

No problem, you were kinda mobbed most of the time 😄

alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 21, 2018
@alexcrichton
rustc: Rewrite platform intrinsics crate
b890d51 
This commit rewrites (updates) the `rustc_platform_intrinsics` crate
in this repository. This crate was previously procedurally generated
from a number of JSON files and was intended to be the basis for how we
bind SIMD in Rust. This crate is quite old at this point and is actually
only used very little. The `stdsimd` crate and `std::arch` modules are
the "new" way for stabilizing SIMD intrinsics.

Despite this, however, the crate isn't useless! Most LLVM intrinsics are
called directly in the `stdsimd` crate using the `link_llvm_intrinsics`
feature, but not all intrinsics can be expressed directly in Rust.
Instead some intrinsics, notably those that return LLVM aggregates, need
to be compiler-defined to ensure they're declared with the correct
signature.

A good example of this is the x86 rdrand/rdseed family of LLVM
intrinsics, all of which return an aggregate of some form. The stdsimd
functions which provide these x86 intrinsics link to functions defined
by this crate, so we need to keep those!

Adding all that together, this commit performs the following tasks:

* Deletes the now-outdated `src/etc/platform-intrinsics` infrastructure
* Deletes all `src/librustc_platform_intrinsics/*` intrinsics that are
  otherwise compatible to be defined in Rust itself. This includes
  everything that doesn't deal with aggregates basically. Empty
  architecture files are removed.
* The style of `src/librustc_platform_intrinsics/` was updated slightly
  to make it a bit easier to handwrite. The `x86` rdrand/rdseed
  intrinsics and some `aarch64` intrinsics which return aggregates have
  all survived to continue to be defined by this crate.

Closes rust-lang#41885
@alexcrichton alexcrichton mentioned this issue Dec 21, 2018
Closed
@steveklabnik
Copy link
Member

Triage: #57416 fixed this bug 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@steveklabnik @alexcrichton @theduke @laumann @aturon @F001 @Mark-Simulacrum @tommyip