Add `tag_for_variant` query by jswrenn · Pull Request #122784 · rust-lang/rust

jswrenn · 2024-03-20T18:14:42Z

This query allows for sharing code between rustc_const_eval and rustc_transmutability. It's a precursor to a PR I'm working on to entirely replace the bespoke layout computations in rustc_transmutability.

r? @compiler-errors

rustbot · 2024-03-20T18:14:51Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung

I went over the interpreter changes only.

RalfJung · 2024-03-20T19:17:53Z

compiler/rustc_const_eval/src/interpret/discriminant.rs

-                tag_field,
-                ..
-            } => {
+        let (tag, tag_field) = match self.tag_for_variant(dest.layout().ty, variant_index)? {


Seems like only one of the match arms falls through. I think the code would become more readable if you just took the code after the match and put it into that arm. Then you can remove the return, the match becomes the tail expression.

RalfJung · 2024-03-20T19:18:59Z

compiler/rustc_const_eval/src/interpret/discriminant.rs

 use super::{ImmTy, InterpCx, InterpResult, Machine, Readable, Scalar, Writeable};

+/// The tag of an enum discriminant.
+pub(crate) enum Tag {


This enum is only useful for writing the tag, not for reading it. I feel like that should somehow be reflected? At least in the docs, maybe in the name. In MiniRust we'd call this the "tagger" for this variant.

Done, by removing Tag altogether.

RalfJung · 2024-03-20T19:20:47Z

compiler/rustc_const_eval/src/interpret/discriminant.rs

+    /// The variant is tagged.
+    Tagged { tag: ScalarInt, tag_field: usize },
+    /// No tag; the variant is identified by its validity.
+    Untagged,


I think Untagged is the name you should use for Single-encoded variants.

Not sure what to do with the case of "no tagging needed as the value is outside the niche and that's enough". One option would be to just merge that case with Untagged. In both cases, there's nothing to be done for writing the tag.

Does the transmutability analysis need to distinguish these?

Does the transmutability analysis need to distinguish these?

Transmutability analysis doesn't, but write_discriminant does: it has an extra validity check in the Untagged case.

Yeah it does the read of the discriminant to verify that "doing nothing" makes sense. But we can just do that for Single-encoded enums as well.

Ah, that simplifies things quite a bit, then. I've updated this PR to forego the the Tag enum altogether in favor of Option<(tag_value, tag_field)>, and made the suggested changes to the match.

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

This query allows for sharing code between `rustc_const_eval` and `rustc_transmutability`. Also moves `DummyMachine` to `rustc_const_eval`.

rustbot · 2024-03-22T17:02:00Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

jswrenn · 2024-03-22T17:03:59Z

compiler/rustc_const_eval/src/const_eval/dummy_machine.rs

This was extracted wholesale from compiler/rustc_mir_transform/src/dataflow_const_prop.rs.

Ah, great, I wanted this to be moved for a while. :)

compiler-errors · 2024-03-22T18:20:46Z

@bors r+

bors · 2024-03-22T18:20:48Z

📌 Commit 2de9010 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics) - rust-lang#122195 (Note that the caller chooses a type for type param) - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish) - rust-lang#122784 (Add `tag_for_variant` query) - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`) - rust-lang#122873 (Merge my contributor emails into one using mailmap) - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups) - rust-lang#122888 (add a couple more tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122784 - jswrenn:tag_for_variant, r=compiler-errors Add `tag_for_variant` query This query allows for sharing code between `rustc_const_eval` and `rustc_transmutability`. It's a precursor to a PR I'm working on to entirely replace the bespoke layout computations in `rustc_transmutability`. r? `@compiler-errors`

RalfJung · 2024-03-23T09:37:36Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs


+/// Computes the tag (if any) for a given type and variant.
+#[instrument(skip(tcx), level = "debug")]
+pub fn tag_for_variant_provider<'tcx>(


This isn't really an "eval" query, so mod.rs may have been a better place... but overall, 🤷 not that important

tag_for_variant follow-ups Follow-up to rust-lang#122784, mostly to clarify the doc comment.

Rollup merge of rust-lang#122933 - RalfJung:tag_for_variant, r=oli-obk tag_for_variant follow-ups Follow-up to rust-lang#122784, mostly to clarify the doc comment.

tag_for_variant follow-ups Follow-up to rust-lang#122784, mostly to clarify the doc comment.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics) - rust-lang#122195 (Note that the caller chooses a type for type param) - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish) - rust-lang#122784 (Add `tag_for_variant` query) - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`) - rust-lang#122873 (Merge my contributor emails into one using mailmap) - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups) - rust-lang#122888 (add a couple more tests) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned compiler-errors Mar 20, 2024

rustbot added 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. labels Mar 20, 2024

RalfJung reviewed Mar 20, 2024

View reviewed changes

jswrenn force-pushed the tag_for_variant branch from 9179e1b to 5342c6a Compare March 20, 2024 20:18

jswrenn requested a review from RalfJung March 20, 2024 20:21

Add tag_for_variant query

2de9010

This query allows for sharing code between `rustc_const_eval` and `rustc_transmutability`. Also moves `DummyMachine` to `rustc_const_eval`.

jswrenn force-pushed the tag_for_variant branch from 5342c6a to 2de9010 Compare March 22, 2024 17:01

jswrenn commented Mar 22, 2024

View reviewed changes

compiler-errors approved these changes Mar 22, 2024

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 Mar 22, 2024

matthiaskrgr mentioned this pull request Mar 22, 2024

Rollup of 8 pull requests #122900

Merged

bors merged commit 96be3e7 into rust-lang:master Mar 23, 2024

rustbot added this to the 1.79.0 milestone Mar 23, 2024

RalfJung reviewed Mar 23, 2024

View reviewed changes

RalfJung mentioned this pull request Mar 23, 2024

tag_for_variant follow-ups #122933

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024

Rollup merge of rust-lang#122933 - RalfJung:tag_for_variant, r=oli-obk

fce8039

tag_for_variant follow-ups Follow-up to rust-lang#122784, mostly to clarify the doc comment.

RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024

Rollup merge of rust-lang#122933 - RalfJung:tag_for_variant, r=oli-obk

f98bbe6

tag_for_variant follow-ups Follow-up to rust-lang#122784, mostly to clarify the doc comment.

Uh oh!

Conversation

jswrenn commented Mar 20, 2024

Uh oh!

rustbot commented Mar 20, 2024

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rustbot commented Mar 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Mar 22, 2024

Uh oh!

bors commented Mar 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants