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

Plugin provided "intrinsics". #52136

Closed

Conversation

DiamondLovesYou
Copy link
Contributor

@DiamondLovesYou DiamondLovesYou commented Jul 7, 2018

Allows plugins to define functions which are only expanded by the plugin after all type checking, inference, etc etc, ie only once codegen encounters a call to the intrinsic. In this initial version, the codegen crate is reponsible for calling the plugin intrinsic codgen trait and codegening the resulting extra MIR statements. Plugins are limited to mir::StatementKind and those contained therein, so they can't insert extra basic blocks, or insert calls to arbitraty functions. This means
the compiler can still reason about what is reachable.

The form of the interface with the plugins is slightly different than what was proposed in #51623.

I understand the desired path is to do an RFC for these sort of things. I haven't started one, and honestly didn't plan to due to time required (and it really doesn't need an RFC; if one would like to see a change,
make the change!), but considering that it'll create a common place to discuss this, maybe I will.

r? @eddyb Probably others too?

Allows plugins to define functions which are only expanded by the plugin
after all type checking, inference, etc etc, ie only once codegen
encounters a call to the intrinsic. In this initial version, the codegen
crate is reponsible for calling the plugin intrinsic codgen trait and
codegening the resulting extra MIR statements. Plugins are limited to
`mir::StatementKind` and those contained therein, so they can't insert
extra basic blocks, or insert calls to arbitraty functions. This means
the compiler can still reason about what is reachable.

The form of the interface with the plugins is slightly different than
what was proposed in rust-lang#51623.
Additionally, signature checking is added.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2018
@kennytm kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 7, 2018
@eddyb
Copy link
Member

eddyb commented Jul 13, 2018

The plugin interface is slated for removal, so it's not the best fit for this.
Have you considered having just a custom attribute on function declarations (or even on foreign function imports in extern {...}), to use within a fork of librustc_codegen_llvm?
cc @rust-lang/compiler @alexcrichton

@alexcrichton
Copy link
Member

Yes I agree that avoiding plugins would be best in this situation, ideally we'd eventually remove them one day.

@DiamondLovesYou
Copy link
Contributor Author

I'm not sure avoiding plugins is even possible, particularly for what I'm trying to achieve (run this closure on the GPU, but without a massive maintenance burden), and minus forced adoption I don't see that changing. I will have to design single-source-ness basically from the ground up, if possible at all.

Attaching attributes, or extern { ... } won't work for closures. And I don't want to fork librustc_codegen_llvm, because that's worse than just allowing plugins (literally, one would have to tag a rust nightly and update codegen to that. how is that better than just using a plugin, with the (possible) power of plugins?). Getting the DefId using a compiler plugin really is the best way to get the info needed to support them.

Is there a log/etc of the discussions for/against removing plugins?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 13, 2018

Couldn't we do everything from this PR except for exposing the API in the plugin API? This way you could write a custom driver and inject your intrinsics this way.

@nagisa
Copy link
Member

nagisa commented Jul 13, 2018

Is there a log/etc of the discussions for/against removing plugins?

If there are any logs, they are very scattered around and nobody collects them. The only instance of anybody arguing for plugins I remember was @nbp during the inception phases of holyjit. Their consideration to use MIR plugins for holyjit led to removal of MIR plugin infrastructure and this removal left a bad aftertaste for all parties involved. In the end holyjit being written as a driver ultimately was an overall better solution anyway.

The usual argument against the plugin infrastructure is the necessity to provide some sort of stable API for the plugins. That would unnecessarily hinder the development of the compiler and we (the compiler team) are not ready to pay the price here yet (if ever). Stable API is already an extremely difficult thing to achieve at a libsyntax/AST level – IIRC somebody is semi-actively working on that and we still don’t have anything there. Stable API at almost a codegen level is a few levels above that.

I really doubt we will take any PR that adds anything resembling external plugin support at this point, except maybe for prototyping of features slated for eventual inclusion into rustc.

@nagisa
Copy link
Member

nagisa commented Jul 13, 2018

What I would try doing is generalising the intrinsic codegen stuff enough that it could be exposed at the driver level and then trying to land that.

@eddyb
Copy link
Member

eddyb commented Jul 13, 2018

@DiamondLovesYou I'm confused, are you trying to treat user-supplied closures as intrinsics?
I was expecting you'd have a library with intrinsics, like @MaikKlein needs for their rlsl crate.

literally, one would have to tag a rust nightly and update codegen to that. how is that better than just using a plugin, with the (possible) power of plugins?

Doing anything other than that, for any code that interacts with the compiler, is irresponsible IMO.
The only thing you get out of plugins (or custom drivers, depending) is some amount of code reuse.

I agree with @nagisa, if you can hook intrinsics through the query engine, especially if you can reuse the MIR shim API for lowering these intrinsics to what MIR already supports, that'd be good.

@bors
Copy link
Contributor

bors commented Jul 14, 2018

☔ The latest upstream changes (presumably #52266) made this pull request unmergeable. Please resolve the merge conflicts.

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Jul 16, 2018

@oli-obk Sure, but that seems like a waste, as then there is an extra crate dependency that cargo is unaware of. Would the wrapper be used to compile downstream crates? Downstream crates would have to know to use the required wrapper. Also, what if, eg, my project and holyjit were used (where both projects had their own wrapper)? Forcing the use of a wrapper is incompatible with this.

@nagisa Interesting. But I don't agree that removing plugins would be a better direction, as such a driver would be basically identical to the upstream driver, exect for the plugin bit; essentially, the way the plugin thingy worked but without cargo. The plugin is a dependency, not, idk, something else. Thus is best managed by cargo.

To be clear, this PR is not designed to support the part of compilation that uses libsyntax/friends/AST. It is specifically for after typeck/borrowck/mir optimization/monomorphization, thus after parsing/macro expansion/etc (I do not want to have to implement Rust syntax/blah/blah stuff independently: stuff is difficult and time consuming). Removal of those parts won't affect this PR, or my desired use cases. Nuke the syntax/AST plugin stuff, as far as I'm concerned 😄

holyjit could actually use this plugin interface to do basically same thing as I do: load the MIR at runtime, and then JIT that using their codegen engine. Then they wouldn't need the macro. Granted, not without significant changes to how holyjit is used. It looks like holyjit adds a instrumentation MIR pass in its driver, so maybe not. Idk.

@eddyb No no no that would be crazy, the user supplied closures and the plugin intrinsics are separate. Here is an example (I know there should extra bounds, ie closure data needs to implement NumaSend (magical for now, just allows fast de-/serializing inner memory regions, such as in Box<T>), also the arguments/return types need NumaSend. Not to mention the unsizing of the upvars needs work. And is Fn too strict? But hopefully you'll get the picture):

use traits::NumaSend;

// roughly corresponds to a DefId in `rustc`.
#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
pub struct KernelId {
  pub crate_name: &'static str,
  pub crate_hash_hi: u64,
  pub crate_hash_lo: u64,
  pub index: u64,
}

mod intrinsics {
  use super::KernelId;
  use traits::NumaSend;

  // These are provided by the plugin:
  extern "rust-intrinsic" {
    pub fn kernel_id_for<'upvar, F, Args, Ret>(f: &'upvar F)
      -> (&'static str, u64, u64, u64)
      where F: Fn<Args, Output=Ret>;

    pub fn kernel_upvars<'upvar, F, Args, Ret>(f: &'upvar F)
      -> &'upvar [&'upvar NumaSend]
      where F: Fn<Args, Output=Ret>;
  }
}

#[derive(Debug, Copy, Clone)]
pub struct KernelInfo<'upvar> {
  pub id: KernelId,
  pub upvars: &'upvar [&'upvar NumaSend],
}

pub fn kernel_info_for<'upvar, F, Args, Ret>(f: &'upvar F) -> KernelInfo<'upvar>
  where F: Fn<Args, Output=Ret>
{
  let (crate_name, crate_hash_hi,
    crate_hash_lo, index) = unsafe {
    intrinsics::kernel_id_for(f)
  };

  // TODO
  const UPVARS: &'static [&'static NumaSend] = &[];

  let id = KernelId {
    crate_name,
    crate_hash_hi,
    crate_hash_lo,
    index,
  };

  KernelInfo {
    id,
    upvars: UPVARS,
  }
}

Then, one will then be able to call kernel_info_for like:

let f = |param: u64| -> Result<(), Error> {
  // ..
};
let def_id_like = kernel_info_for(&f);

And use def_id_like to get the real DefId using the metadata from the host. Then go nuts.

It doesn't only save a small amount of code due to reuse. This basically reuses the entire compiler! Without any syntax extension. Which is almost a requirement, as this work was/will be done in my free time. Among the other 8 billion projects I always adopt (please, send help).

To be clear, I never expected API stability, I love the gradual improvements! To handle the instability, I planned to semi-regularly "tag" a specific nightly date (so I can tell uses to use, eg, nightly-2018-07-15-x86_64-unknown-linux-gnu from rustup) and update the relevant project crates to use the rustc_private API from that.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 16, 2018

Sure, but that seems like a waste, as then there is an extra crate dependency that cargo is unaware of.

If you are writing a plugin, that is also true.

Would the wrapper be used to compile downstream crates?

Yes

Downstream crates would have to know to use the required wrapper.

No, that's automatic when you use RUSTC_WRAPPER

Also, what if, eg, my project and holyjit were used (where both projects had their own wrapper)? Forcing the use of a wrapper is incompatible with this.

If you want to use both, then you should create a driver that reflect that. To be honest, this case is exactly why we should not be using plugins. Combining MIR passes in a decentralized way is a recipe for disaster. Their interactions are not clear at all.


General point: I was of a similar opinion like you when we added the clippy driver and made it the preferred default instead of the plugin interface... Until I saw it in action. I have written 3 drivers, two of which are used very frequently and I have to say it's very convenient to have full control.

Removal of those parts won't affect this PR, or my desired use cases. Nuke the syntax/AST plugin stuff, as far as I'm concerned

Uhm. Not sure how serious your statement is, but if it was meant seriously, that's an argument for "Not having a MIR pass plugin won't affect my desired use cases. Don't ever readd it as far as I'm concerned".

General point 2: You can write a driver that exposes a MIR-pass plugin interface entirely to your specifications.

@nbp
Copy link
Contributor

nbp commented Jul 18, 2018

As @nagissa mentioned, The decision for removing plugins was made over IRC on the rustc channel, after asking how I could get a plugin working with the MIR, at the last stage of the pipeline.

For holyjit, the main advantages of using plugin over a driver was:

  • We had sparse documentation about making syntax plugins for making procedural macros.
  • Cargo understand what a plugin is, and can use it.
  • Multiple plugins could be used at the same time (composition; for example using clippy + holyjit + …).
  • The compiler is still named rustc. (not having to argue about using a forked version of rustc is simpler to advocate)

I honestly think that all plugins can be removed, if all the previous points are addressed.

I do not think the argument about the stability of the API is a good one compared to the driver. The driver API is as unstable as any plugin API would be. However, the driver API is cleaner and much more expressive than adding new functions to a plugin interface.

@DiamondLovesYou :

holyjit could actually use this plugin interface to do basically same thing as I do […]

holyjit aims at using the default code generator to do a lot of compile-time optimization, to get as much performance as possible from the non-JIT-ed code. The macros are used to annotate the parts which should be given to the JIT compiler, as a JIT has a lot of space/time overhead.

@pietroalbini
Copy link
Member

Ping from triage! Maybe the compiler team can make a decision at their meeting?

@DiamondLovesYou
Copy link
Contributor Author

@ndp (To your first point) Okay, but if I may lead one on a thought experiment, suppose we remove all plugin interfaces, and require projects use a driver for any sort of customization. The use cases for these customization's won’t just go away, and, as you said, projects will want to be able to combine these customizations. So someone creates an uberdriver which handles taking the various desired customizations and applying them to the instance of the compiler it creates and manages. But now we have another project which will require API synchronization with the compiler (granted, each of the plugins will have to have close/exact same rustc versions, so maybe not a practical issue), which is, at best, worse than if plugin development happened in Rust proper (only the plugins can have API incompatibilities). Regardless of what driver is used, dynamically loading the plugin into the driver will be required unless you what to require that the driver is recompiled for every set of plugins (not even sure that this could work without help from outside the driver), so something akin to the Registry structure will probably be used so that the driver can resolve and call a single function and get all the information it needs in one go.

Making Cargo understand (more than it already does) plugins has interesting implications. Sadly, I object to moving the driver into Cargo, as I've run into mysterious segfaults as a result of stuff that one can get away with when LLVM assertions are off (especially while developing the address space PR). For that reason, I'd argue that the compiler must be in a separate process from Cargo. The current rustc cli is already an API of sorts, so what's to be gained vs other things (limited time and all) from working on "fixing" this?

Lastly, @ndp's last point pretty much requires that the plugin loading/interfaces remain in Rust proper. Either that or it's a sanctioned driver, but that's basically the same thing.

I'm all for progress, but where is the solution to replace plugins and what they (the plugins) could (not just can) do? Requiring a wrapping driver is terrible overkill when most of the compilation process is left unchanged! It also seems to me that the requirements are best satisfied by leaving the existing plugin infra in place. Can we keep allowing plugins and their development until this magical solution arrives? And btw I'm all for adding the ability to insert custom MIR passes in plugins, a previous attempt at single-source cross compilation used a pass to extract the info needed (before I understood that monomorphization's really happened after all of the MIR passes...).

@ndp (regarding your second point) Interesting. I'm not that familiar with JIT tech, but that actually sounds close to what I'm doing, though I'm not concerning myself with time/space overheads, yet (its designed for big-ish iron anyway). The JIT-ing isn't required for me, but allows optimizing the code to the target (and is/will be cached).

@pnkfelix
Copy link
Member

compiler team briefly discussed at meeting. We definitely think this PR needs an RFC. (Also, a number of compiler team members want plugins to go away if possible.)

@pnkfelix pnkfelix closed this Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.