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

[EXPERIMENT] Reduction of monomorphization load through type erasure #85308

Closed
wants to merge 2 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented May 14, 2021

This is an early proposition in response to #77960 through type-erasure of a function's MIR. This is still an experiment, and there are probably a lot of edge cases I did not cover. Any weird addition to the test cases is welcomed 😄

Here is the description I gave in #77960 (comment). Consider a function

fn foo<T>(a: M<T>) -> R<T> {
  let c: C<T> = T::CONST;
  // stuff with T
  T::bar(a, c)
}

During MIR analyses, the trampoline and the callee version can be computed generically:

  • gather all constants inside MIR body, and replace them by appropriately-typed parameters;
  • gather all functions, and replace them by function pointer parameters.
fn foo<T>(a: M<T>) -> R<T> {
  let _2: C<T> = T::CONST;
  let _3: fn(M<T>, C<T>) -> R<T> = T::bar;
  foo_impl<T>(a, _2, _3)
}

fn foo_impl<T>(a: M<T>, c: C<T>, _3: fn(M<T>, C<T>) -> R<T>) -> R<T> {
  // stuff with T
  _3(a, c)
}

The next step is to only have one instance of foo_impl. In rustc parlance, I introduced an new variant InstanceDef::ErasedShim to encode it. It is resolved when some a MIR call terminator has the new flag erased = true. In order to define the substitutions for this instance, we replace all the type parameters by u8. The code becomes:

type T_impl = u8; // We do not need it to satisfy any trait except Sized.

fn foo<T>(a: M<T>) -> R<T> {
  let _2: C<T> = T::CONST;
  let _3: fn(M<T>, C<T>) -> R<T> = T::bar;
  transmute(foo_impl(transmute(a), transmute(_2), transmute(_3)))
}

fn foo_impl(a: M<T_impl>, c: C<T_impl>, _3: fn(M<T_impl>, C<T_impl>) -> R<T_impl>) -> R<T_impl> {
  // stuff with T_impl
  _3(a, c)
}

For this transformation to be sound, we need to check that all those transmutes are safe.

The problem is that Rust layouts all monomorphized types independently: in general M<T> and M<T_impl> have no reason to have a similar layout if T != U. In special cases, this can happen, for instance if M<T> only points to sized T through a reference.

As a consequence, we need to check that M<T> and M<T_impl> and all recursively accessible types have the same layout. Likewise for C<T> and R<T>. If any such test fails, the type-erasure is unsound, we need to abort and go back to full monomorphization. Actually, we should be able to restrict the check to all types that are involved in the MIR: types of locals, of accessed fields, of dereferenced pointers...

The implementation actually does this in reverse. First, we check the layout in the function gather_types, and bail out if there is an incompatibility. Second, we gather and replace all the constants in the MIR. Handling drops requires to convert them into calls to drop_in_place in the same step. Third, we build the caller MIR. Finally, we erased all the type parameters in the callee MIR.

cc @jumbatm @jyn514

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking rustc_passes v0.0.0 (/checkout/compiler/rustc_passes)
    Checking rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
    Checking rustc_typeck v0.0.0 (/checkout/compiler/rustc_typeck)
    Checking rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
error[E0063]: missing field `erased` in initializer of `rustc_middle::mir::TerminatorKind<'_>`
  --> compiler/rustc_mir/src/dataflow/framework/tests.rs:37:9
37 |         mir::TerminatorKind::Call {
37 |         mir::TerminatorKind::Call {
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^ missing `erased`

error[E0063]: missing field `erased` in initializer of `rustc_middle::mir::TerminatorKind<'_>`
  --> compiler/rustc_mir/src/dataflow/framework/tests.rs:50:9
50 |         mir::TerminatorKind::Call {
50 |         mir::TerminatorKind::Call {
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^ missing `erased`

error[E0063]: missing field `erased` in initializer of `rustc_middle::mir::TerminatorKind<'_>`
    |
150 |             TerminatorKind::Call {
150 |             TerminatorKind::Call {
    |             ^^^^^^^^^^^^^^^^^^^^ missing `erased`
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0063`.
error: could not compile `rustc_mir`
error: could not compile `rustc_mir`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "--all-targets" "-p" "rustc-main" "-p" "rustc_codegen_ssa" "-p" "rustc_ast" "-p" "rustc_lexer" "-p" "rustc_symbol_mangling" "-p" "rustc_apfloat" "-p" "rustc_serialize" "-p" "rustc_macros" "-p" "rustc_errors" "-p" "rustc_lint_defs" "-p" "rustc_target" "-p" "rustc_index" "-p" "rustc_attr" "-p" "rustc_ast_pretty" "-p" "rustc_feature" "-p" "rustc_data_structures" "-p" "rustc_graphviz" "-p" "rustc_hir" "-p" "rustc_incremental" "-p" "rustc_middle" "-p" "rustc_type_ir" "-p" "rustc_query_system" "-p" "rustc_arena" "-p" "rustc_fs_util" "-p" "rustc_span" "-p" "rustc_session" "-p" "rustc_driver" "-p" "rustc_interface" "-p" "rustc_ast_passes" "-p" "rustc_query_impl" "-p" "rustc_expand" "-p" "rustc_trait_selection" "-p" "rustc_parse_format" "-p" "rustc_infer" "-p" "rustc_builtin_macros" "-p" "rustc_resolve" "-p" "rustc_ty_utils" "-p" "rustc_traits" "-p" "rustc_privacy" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_passes" "-p" "rustc_ast_lowering" "-p" "rustc_mir_build" "-p" "rustc_save_analysis" "-p" "rustc_typeck" "-p" "rustc_plugin_impl" "-p" "rustc_lint" "-p" "rustc_error_codes" "-p" "rustc_mir" "-p" "coverage_test_macros" "-p" "rustc_parse" "-p" "rustc_metadata" "-p" "rustc_hir_pretty" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:01:34

@@ -425,6 +433,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// recurse with concrete function
self.eval_fn_call(drop_fn, caller_abi, &args, ret, unwind)
}
ty::InstanceDef::ErasedShim(_) => {
bug!("Interpreting type-erased functions is not implemented.")
Copy link
Member

Choose a reason for hiding this comment

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

If this actually lands, I'd appreciate help making this code work in Miri. :)
Also, if the CTFE engine does not support these calls, we must make sure they are rejected in const fn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this ever lands, these calls will only be created by a MIR-opt pass. As a consequence, they should not appear in a const fn.

Copy link
Member

Choose a reason for hiding this comment

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

Even optimized code must, IMO, be executable by the Miri engine -- and indeed it is possible to run Miri on optimized MIR with -Zmir-opt-level=4. The code here is relevant well beyond const fn. So while it is okay to temporarily land a feature without Miri engine support, this is always a bug that should be fixed eventually.

Copy link
Member

Choose a reason for hiding this comment

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

But I guess this means we indeed don't have to reject anything in const fn. However, we only recently stopped optimizing the MIR used by CTFE, and we might revert that decision at some point, so there's a danger of some lock-in here.

@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

I think this definitely needs an MCP and someone with more knowledge in this area to review.

@jackh726 jackh726 removed their assignment Jun 1, 2021
@jackh726 jackh726 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 Jun 1, 2021
@bors
Copy link
Contributor

bors commented Jul 16, 2021

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

@Mark-Simulacrum
Copy link
Member

Going to go ahead and close as an inactive "experiment", but of course feel free to reopen if you make further progress or this becomes ready for review -- I agree with @jackh726 that a MCP may be warranted.

@jieyouxu jieyouxu added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants