rustc_target: `TyAndLayout::field` should never error. #88337

eddyb · 2021-08-25T18:01:43Z

This refactor (making TyAndLayout::field return TyAndLayout without any Result around it) is based on a simple observation, regarding TyAndLayout::field:

If cx.layout_of(ty) succeeds (for some cx and ty), then .field(cx, i) on the resulting TyAndLayout should always succeed in computing cx.layout_of(field_ty) (where field_ty is the type of the ith field of ty).

The reason for this is that no matter which field is chosen, cx.layout_of(field_ty) will have already been computed, as part of computing cx.layout_of(ty), as we cannot determine the layout of any type without considering the layouts of all of its fields.

And so it should be fine to turn any errors into ICEs, since they likely indicate a cx mismatch, or some other edge case that is due to a compiler bug (as opposed to ever being an user-facing error).

Each commit should probably be reviewed separately, though note that there's some where clauses (in rustc_target::abi::call::*) that change in most commits.

cc @nagisa @oli-obk

rust-highfive · 2021-08-25T18:01:46Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-08-25T18:01:47Z

r? @wesleywiser

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

nagisa · 2021-08-26T08:35:20Z

r? @nagisa

eddyb · 2021-08-27T11:09:26Z

@bors try @rust-timer queue

rust-timer · 2021-08-27T11:09:27Z

Awaiting bors try build completion.

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

bors · 2021-08-27T11:09:33Z

⌛ Trying commit cfabbc654c3b77e927f3eadaebd95497e433c066 with merge 04674fabacf07ea0451047603a002a2d7a389cf2...

bors · 2021-08-27T12:28:56Z

☀️ Try build successful - checks-actions
Build commit: 04674fabacf07ea0451047603a002a2d7a389cf2 (04674fabacf07ea0451047603a002a2d7a389cf2)

rust-timer · 2021-08-27T12:28:58Z

Queued 04674fabacf07ea0451047603a002a2d7a389cf2 with parent dfd6306, future comparison URL.

rust-timer · 2021-08-27T15:20:10Z

Finished benchmarking try commit (04674fabacf07ea0451047603a002a2d7a389cf2): 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

bjorn3 · 2021-08-27T16:36:16Z

A 1.25% regression for coercions-debug incr-patched: add static arr item, but otherwise all changes are easily within noise.

eddyb · 2021-08-27T17:40:53Z

A 1.25% regression for coercions-debug incr-patched: add static arr item, but otherwise all changes are easily within noise.

That regression is pretty surprising in many ways. It doesn't match any patterns in other benchmarks and the query view shows things like typeck and LLVM_passes. If I had to guess, I'd say it's a freak non-deterministic accident.

compiler/rustc_codegen_ssa/src/mir/block.rs

nagisa · 2021-08-28T15:26:50Z

compiler/rustc_middle/src/ty/layout.rs

-        enum TyMaybeWithLayout<C: LayoutOf> {
-            Ty(C::Ty),
-            TyAndLayout(C::TyAndLayout),
+    fn ty_and_layout_field(this: TyAndLayout<'tcx>, cx: &C, i: usize) -> TyAndLayout<'tcx> {


Hm, would there be value in making sure the naming between this and the ty_and_layout_FOR_variant is consistent?

The prefix ty_and_layout_ is consistent between the two, IIRC. If you mean TyAndLayout::for_variant should be renamed, I guess as_variant or variant_downcast might be okay, but I don't have a strong preference.

Yeah I mean ty_and_layout_for_variant+ty_and_layout_for_field or just ty_and_layout_variant+ty_and_layout_field.

nagisa · 2021-08-28T15:28:27Z

r=me up to you to decide if you want to do anything about the inline comments.

eddyb · 2021-08-29T21:47:28Z

@bors r=nagisa

I resolved the first comment, but my reasoning for not changing the ty_and_layout_{for_variant,field} methods that provide TyAndLayout::{for_variant,field} (as stated in #88337 (comment)) is that they're consistent right now (via TyAndLayout:: <-> ty_and_layout_).

bors · 2021-08-29T21:47:30Z

📌 Commit 78778fc has been approved by nagisa

bors · 2021-08-29T22:54:29Z

⌛ Testing commit 78778fc with merge 9556d7a...

bors · 2021-08-30T01:35:10Z

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

rust-highfive · 2021-08-30T01:35:39Z

📣 Toolstate changed by #88337!

Tested on commit 9556d7a.
Direct link to PR: #88337

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

@oli-obk

Tested on commit rust-lang/rust@9556d7a. Direct link to PR: <rust-lang/rust#88337> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @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`

rust-highfive assigned wesleywiser Aug 25, 2021

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

eddyb mentioned this pull request Aug 25, 2021

rustc_target: route LayoutOf through TyAbiInterface. #88338

Closed

rust-highfive assigned nagisa and unassigned wesleywiser Aug 26, 2021

eddyb added 3 commits August 27, 2021 13:09

rustc_target: add lifetime parameter to LayoutOf.

83d986a

rustc_target: rename TyAndLayoutMethods to TyAbiInterface.

8486571

rustc_target: require TyAbiInterface in LayoutOf.

87d1fb7

eddyb force-pushed the field-failure-is-not-an-option branch from b6d4f6c to cfabbc6 Compare August 27, 2021 11:01

eddyb marked this pull request as ready for review August 27, 2021 11:08

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

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

nagisa reviewed Aug 28, 2021

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

nagisa reviewed Aug 28, 2021

View reviewed changes

eddyb added 2 commits August 30, 2021 00:44

rustc_target: TyAndLayout::field should never error.

8e6d126

rustc_target: remove LayoutOf bound from TyAbiInterface.

78778fc

eddyb force-pushed the field-failure-is-not-an-option branch from cfabbc6 to 78778fc Compare August 29, 2021 21:44

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 Aug 29, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2021

bors merged commit 9556d7a into rust-lang:master Aug 30, 2021

rustbot added this to the 1.56.0 milestone Aug 30, 2021

rust-highfive mentioned this pull request Aug 30, 2021

miri no longer builds after rust-lang/rust#88337 #88473

Closed

bors mentioned this pull request Aug 30, 2021

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

Merged

eddyb deleted the field-failure-is-not-an-option branch August 30, 2021 04:44

ricky26 mentioned this pull request Aug 30, 2021

Update LLVM submodule #88395

Merged

eddyb mentioned this pull request Aug 30, 2021

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

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

rustc_target: `TyAndLayout::field` should never error. #88337

rustc_target: `TyAndLayout::field` should never error. #88337

eddyb commented Aug 25, 2021 •

edited

Loading

rust-highfive commented Aug 25, 2021

rust-highfive commented Aug 25, 2021

nagisa commented Aug 26, 2021

eddyb commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bors commented Aug 27, 2021

bors commented Aug 27, 2021

rust-timer commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bjorn3 commented Aug 27, 2021

eddyb commented Aug 27, 2021

nagisa Aug 28, 2021

eddyb Aug 28, 2021

nagisa Aug 28, 2021

nagisa commented Aug 28, 2021

eddyb commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 30, 2021

rust-highfive commented Aug 30, 2021

rustc_target: TyAndLayout::field should never error. #88337

rustc_target: TyAndLayout::field should never error. #88337

Conversation

eddyb commented Aug 25, 2021 • edited Loading

rust-highfive commented Aug 25, 2021

rust-highfive commented Aug 25, 2021

nagisa commented Aug 26, 2021

eddyb commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bors commented Aug 27, 2021

bors commented Aug 27, 2021

rust-timer commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bjorn3 commented Aug 27, 2021

eddyb commented Aug 27, 2021

nagisa Aug 28, 2021

Choose a reason for hiding this comment

eddyb Aug 28, 2021

Choose a reason for hiding this comment

nagisa Aug 28, 2021

Choose a reason for hiding this comment

nagisa commented Aug 28, 2021

eddyb commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 30, 2021

rust-highfive commented Aug 30, 2021

rustc_target: `TyAndLayout::field` should never error. #88337

rustc_target: `TyAndLayout::field` should never error. #88337

eddyb commented Aug 25, 2021 •

edited

Loading