Provide `layout_of` automatically (given tcx + param_env + error handling). #88499

eddyb · 2021-08-30T17:55:18Z

After #88337, there's no longer any uses of LayoutOf within rustc_target itself, so I realized I could move the trait to rustc_middle::ty::layout and redesign it a bit.

This is similar to #88338 (and supersedes it), but at no ergonomic loss, since there's no funky C: LayoutOf<Ty = Ty> -> Ty: TyAbiInterface<C> generic impl chain, and each LayoutOf still corresponds to one impl (of LayoutOfHelpers) for the specific context.

After this PR, this is what's needed to get trait LayoutOf (with the layout_of method) implemented on some context type:

TyCtxt, via HasTyCtxt
ParamEnv, via HasParamEnv
a way to transform LayoutErrors into the desired error type
- an error type of ! can be paired with having cx.layout_of(...) return TyAndLayout without Result<...> around it, such as used by codegen
- this is done through a new LayoutOfHelpers trait (and so is specifying the type of cx.layout_of(...))

When going through this path (and not bypassing it with a manual impl of LayoutOf), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform.

(EDIT: just noticed that because of the supertrait relationship, you cannot actually implement LayoutOf yourself, the blanket impl fully covers all possible context types that could ever implement it)

Part of the motivation for this shape of API is that I've been working on querifying FnAbi::of_*, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the FnAbi methods in rustc_target, since they're much more tied to rustc concepts.

r? @nagisa cc @oli-obk @bjorn3

rust-highfive · 2021-08-30T17:55:20Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

eddyb · 2021-08-30T17:55:43Z

@bors try @rust-timer queue

rust-timer · 2021-08-30T17:55:44Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-30T17:55:51Z

⌛ Trying commit e9da4729d53073f1d7f785ffe3f05e6b60905743 with merge 5e7403551353c282c2f168ba6ff1b451eecb45a7...

bjorn3 · 2021-08-30T18:30:01Z

compiler/rustc_middle/src/ty/layout.rs

+    fn to_result(self) -> Result<T, Self::Error> {
+        self
+    }
+}


How does this work without specialization and not result in conflicting impls?

As I understand it, trying to unify Result<$0, _> with $0 (to check whether one of the impls applies in the context of the other one) will fail because its sole "solution" is an infinitely nested Result<Result<Result<... - in the literature, I believe this is called an "occurs check".

A crucial component of this, btw, is that MaybeResult is generic over the Ok/T type, instead of having it associated like the Err/Error type - if it was an associated type, it would indeed overlap.

This also shows up when implementing From: impl<T> From<T> for T doesn't involve specialization to avoid conflicting with your own different impls.

bjorn3 · 2021-08-30T18:31:39Z

compiler/rustc_middle/src/ty/layout.rs

+    // `TyCtxt::at`-like APIs to be able to do e.g. `cx.at(span).layout_of(ty)`.
+    #[inline]
+    fn spanned_layout_of(&self, ty: Ty<'tcx>, span: Span) -> Self::LayoutOfResult {
+        let span = if !span.is_dummy() { span } else { self.layout_tcx_at_span() };


Why not directly use layout_tcx_at_span() in layout_of?

Because then I have to either duplicate the rest of the body, or end up with silly codegen that calls layout_tcx_at_span twice.

Overall I don't like the current design (where spanned_layout_of exists), and the way miri handles this (where its tcx field is a TyCtxtAt that it keeps updating) feels superior.

bors · 2021-08-30T19:19:26Z

☀️ Try build successful - checks-actions
Build commit: 5e7403551353c282c2f168ba6ff1b451eecb45a7 (5e7403551353c282c2f168ba6ff1b451eecb45a7)

rust-timer · 2021-08-30T19:19:28Z

Queued 5e7403551353c282c2f168ba6ff1b451eecb45a7 with parent 6cfa773, future comparison URL.

rust-timer · 2021-08-30T20:57:23Z

Finished benchmarking try commit (5e7403551353c282c2f168ba6ff1b451eecb45a7): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

eddyb · 2021-08-30T21:19:29Z

Happy with those perf results (I was worried something might accidentally get worse in the reshuffle, but that doesn't seem to be the case).

…ves.

nagisa · 2021-09-05T15:07:47Z

compiler/rustc_middle/src/ty/layout.rs


+    /// `Span` to use for `tcx.at(span)`, from `layout_of`.
+    // FIXME(eddyb) perhaps make this mandatory to get contexts to track it better?


I can see something like CodegenCx needing to get better at it, but I don't see how having TyCtxt track any sort of span would work? If we made this required, we would have to drop the LayoutOfHelpers impls for TyCtxt and only keep the TyCtxtAt.

Seems reasonable enough, but also sounds like a big undertaking.

nagisa · 2021-09-05T15:13:33Z

compiler/rustc_middle/src/ty/layout.rs

+
+    #[inline]
+    fn layout_tcx_at_span(&self) -> Span {
+        self.tcx.span


Ha, I spent a couple minutes wondering where is there a span in TyCtxt. Took me a while to notice this is an impl on a LayoutCx, not directly TyCtxtAt.

nagisa · 2021-09-05T15:15:07Z

@bors r+

bors · 2021-09-05T15:15:08Z

📌 Commit f53c93c has been approved by nagisa

bors · 2021-09-05T16:14:44Z

⌛ Testing commit f53c93c with merge e2750ba...

bors · 2021-09-05T18:55:26Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing e2750ba to master...

rust-highfive · 2021-09-05T18:55:53Z

📣 Toolstate changed by #88499!

Tested on commit e2750ba.
Direct link to PR: #88499

💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).

@eddyb

Tested on commit rust-lang/rust@e2750ba. Direct link to PR: <rust-lang/rust#88499> 💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).

Provide `layout_of` automatically (given tcx + param_env + error handling). After rust-lang#88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit. This is similar to rust-lang#88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context. After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type: * `TyCtxt`, via `HasTyCtxt` * `ParamEnv`, via `HasParamEnv` * a way to transform `LayoutError`s into the desired error type * an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen * this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`) When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform. (**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it) Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts. r? `@nagisa` cc `@oli-obk` `@bjorn3`

Querify `FnAbi::of_{fn_ptr,instance}` as `fn_abi_of_{fn_ptr,instance}`. *Note: opening this PR as draft because it's based on rust-lang#88499* This more or less replicates the `LayoutOf::layout_of` setup from rust-lang#88499, to replace `FnAbi::of_{fn_ptr,instance}` with `FnAbiOf::fn_abi_of_{fn_ptr,instance}`, and also route them through queries (which `layout_of` has used for a while). The two changes at the use sites (other than the names) are: * return type is now wrapped in `&'tcx` * the value *is* interned, which may affect performance * the `extra_args` list is now an interned `&'tcx ty::List<Ty<'tcx>>` * should be cheap (it's empty for anything other than C variadics) Theoretically, a `FnAbiOfHelpers` implementer could choose to keep the `Result<...>` instead of eagerly erroring, but the only existing users of these APIs are codegen backends, so they don't (want to) take advantage of this. At least miri could make use of this, since it prefers propagating errors (it "just" doesn't use `FnAbi` yet - cc `@RalfJung).` The way this is done is probably less efficient than what is possible, because the queries handle the correctness-oriented API (i.e. the split into `fn` pointers vs instances), whereas a lower-level query could end up with more reuse between different instances with identical signatures. r? `@nagisa` cc `@oli-obk` `@bjorn3`

rust-highfive assigned nagisa Aug 30, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 30, 2021

eddyb mentioned this pull request Aug 30, 2021

rustc_target: route LayoutOf through TyAbiInterface. #88338

Closed

bjorn3 reviewed Aug 30, 2021

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 30, 2021

eddyb added 3 commits September 2, 2021 01:17

rustc_target: move LayoutOf to ty::layout.

4ce933f

ty::layout: implement layout_of automatically as a default method.

1e02262

ty::layout: split LayoutOf into required and (blanket) provided hal…

f53c93c

…ves.

eddyb force-pushed the layout-off branch from e9da472 to f53c93c Compare September 1, 2021 22:17

eddyb mentioned this pull request Sep 1, 2021

Querify FnAbi::of_{fn_ptr,instance} as fn_abi_of_{fn_ptr,instance}. #88575

Merged

nagisa approved these changes Sep 5, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2021

bors merged commit e2750ba into rust-lang:master Sep 5, 2021

rustbot added this to the 1.57.0 milestone Sep 5, 2021

rust-highfive mentioned this pull request Sep 5, 2021

miri no longer builds after rust-lang/rust#88499 #88671

Closed

This was referenced Sep 5, 2021

Add codegen option for branch protection and pointer authentication on AArch64 #88354

Merged

WrappingRange (#88242) follow-up (is_full_for, Scalar: Copy, etc.) #88327

Merged

eddyb deleted the layout-off branch September 5, 2021 22:11

wesleywiser mentioned this pull request Sep 7, 2021

Fix debuginfo for parameters passed via the ScalarPair abi on Windows #88629

Merged

eddyb mentioned this pull request Oct 12, 2021

abi: readjust FnAbis to remove unsupported PassModes, via query hooks. EmbarkStudios/rust-gpu#766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide `layout_of` automatically (given tcx + param_env + error handling). #88499

Provide `layout_of` automatically (given tcx + param_env + error handling). #88499

eddyb commented Aug 30, 2021 •

edited

Loading

rust-highfive commented Aug 30, 2021

eddyb commented Aug 30, 2021

rust-timer commented Aug 30, 2021

bors commented Aug 30, 2021

bjorn3 Aug 30, 2021

eddyb Aug 30, 2021

bjorn3 Aug 30, 2021

eddyb Aug 30, 2021

bors commented Aug 30, 2021

rust-timer commented Aug 30, 2021

rust-timer commented Aug 30, 2021

eddyb commented Aug 30, 2021

nagisa Sep 5, 2021

nagisa Sep 5, 2021

nagisa commented Sep 5, 2021

bors commented Sep 5, 2021

bors commented Sep 5, 2021

bors commented Sep 5, 2021

rust-highfive commented Sep 5, 2021


		/// `Span` to use for `tcx.at(span)`, from `layout_of`.
		// FIXME(eddyb) perhaps make this mandatory to get contexts to track it better?

Provide layout_of automatically (given tcx + param_env + error handling). #88499

Provide layout_of automatically (given tcx + param_env + error handling). #88499

Conversation

eddyb commented Aug 30, 2021 • edited Loading

rust-highfive commented Aug 30, 2021

eddyb commented Aug 30, 2021

rust-timer commented Aug 30, 2021

bors commented Aug 30, 2021

bjorn3 Aug 30, 2021

Choose a reason for hiding this comment

eddyb Aug 30, 2021

Choose a reason for hiding this comment

bjorn3 Aug 30, 2021

Choose a reason for hiding this comment

eddyb Aug 30, 2021

Choose a reason for hiding this comment

bors commented Aug 30, 2021

rust-timer commented Aug 30, 2021

rust-timer commented Aug 30, 2021

eddyb commented Aug 30, 2021

nagisa Sep 5, 2021

Choose a reason for hiding this comment

nagisa Sep 5, 2021

Choose a reason for hiding this comment

nagisa commented Sep 5, 2021

bors commented Sep 5, 2021

bors commented Sep 5, 2021

bors commented Sep 5, 2021

rust-highfive commented Sep 5, 2021

Provide `layout_of` automatically (given tcx + param_env + error handling). #88499

Provide `layout_of` automatically (given tcx + param_env + error handling). #88499

eddyb commented Aug 30, 2021 •

edited

Loading