Store a `MirSource` inside every `Body` #77430

ecstatic-morse · 2020-10-01T21:55:30Z

Resolves #77427.

ecstatic-morse · 2020-10-01T23:14:05Z

@lcnr I don't understand how to use WithOptConstParam. What's going wrong here?

ecstatic-morse · 2020-10-02T02:31:00Z

r? @lcnr I guess. I was expecting this to be a straightforward refactoring, but #74113 has made me a bit frustrated

It feels like we could make WithOptConstParam less invasive when it comes to the *_mir queries. A MIR body either comes from an AnonConst or not, and that doesn't change as we progress through the optimization pipeline. Given that, why does this type infect everything?

lcnr

So WithOptConstParam is used if we ever call type_of for the mir body.

As we call type_of in some annoyingly annoying places, we have to somehow get the DefId of the relevant const param there.
Once we build a Body with the correct WithOptConstParam, we should then be able to use that mir_source in the remaining mir_* queries.

WithOptConstParam infects all of the mir queries as we call optimized_mir during typeck (which is where the cycle errors are the problem) so we have to somehome thread these calls down to mir_built which uses type_of. Not sure if type_of is called in some other mir queries

lcnr · 2020-10-02T08:32:38Z

compiler/rustc_mir_build/src/build/mod.rs

@@ -740,6 +750,7 @@ fn construct_error<'a, 'tcx>(hir: Cx<'a, 'tcx>, body_id: hir::BodyId) -> Body<'t
 impl<'a, 'tcx> Builder<'a, 'tcx> {
    fn new(
        hir: Cx<'a, 'tcx>,
+        def_id: DefId,


Suggested change

def_id: DefId,

def: ty::WithOptConstParam<DefId>,

I think it should be easier to already pass the correct WithOptConstParam into this builder.

That's the part I'm not understanding. What are the invariants of WithOptConstParam? When is it okay to be unknown? Do I have to call try_upgrade manually?

The invariant of WithOptConstParam is that if self.const_param_did.is_some(), then self.const_param_did == tcx.opt_const_param_of(self.did). To prevent cycle errors self.const_param_did should also be Some if it is used during typeck, did is a const argument and we want to know tcx.type_of(did), which we then replace with tcx.type_of(def.def_id_for_type_of()).

You should never have to call try_upgrade except at the start of a query taking ty::WithOptConstParam so that we only evaluate it once.

It is okay to use unknown if we are either done with typeck or tcx.opt_const_param_of(self.did) would/did return None.

Looks like it might be worth it to add a section to the rustc-dev-guide about WithOptConstParam 🤔

Looks like it might be worth it to add a section to the rustc-dev-guide about WithOptConstParam thinking

As I've alluded to, I think time would be better spent better encapsulating this system. I don't particularly want to think about it when writing MIR transformations. I realize I'm being a bit curmudgeonly here and that this is in service of a feature which I'm super excited about (thanks BTW!), but technical debt is a real concern, especially in mir-opt land.

Is there anything we could do to reduce the surface area here? We can discuss this in a separate issue I guess.

Looks like I didn't send the message I wanted to send here 😅

While working on this I experimented with some different approaches and the less invasive ones all ended up being unsound in the face of incremental compilation. So I would be glad - and a bit annoyed 😆 - if you are able to come up with a less invasive solution here, but do think that this will be quite hard to do

I have some ideas, but if you've already explored alternatives, it's unlikely that I'll be able to do better. I've not given up quite yet though 😄.

compiler/rustc_middle/src/mir/mod.rs

lcnr · 2020-10-02T08:40:55Z

compiler/rustc_middle/src/mir/mod.rs

+}
+
+impl<'tcx> MirSource<'tcx> {
+    pub fn item(def_id: DefId) -> Self {


Suggested change

pub fn item(def_id: DefId) -> Self {

pub fn item(def: ty::WithOptParam<DefId>) -> Self {

my personal preference, makes it harder to get this wrong

This isn't mine. I've just moved it from transform/mod.rs.

yeah, I still think that this would be a good change though 🤔 you don't have to do this in this PR

lcnr · 2020-10-02T08:43:36Z

compiler/rustc_mir/src/transform/add_moves_for_packed_drops.rs

-        add_moves_for_packed_drops(tcx, body, src.def_id());
+    fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+        debug!("add_moves_for_packed_drops({:?} @ {:?})", body.source, body.span);
+        add_moves_for_packed_drops(tcx, body, body.source.def_id());


Suggested change

add_moves_for_packed_drops(tcx, body, body.source.def_id());

add_moves_for_packed_drops(tcx, body);

we only use to body source in add_moves_for_packed_drops, don't we?

lcnr · 2020-10-02T08:51:15Z

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs

        let mut opt_finder = RemoveUnneededDropsOptimizationFinder {
            tcx,
            body,
            optimizations: vec![],
-            def_id: source.def_id().expect_local(),
+            def_id: body.source.def_id().expect_local(),


Suggested change

def_id: body.source.def_id().expect_local(),

Can we instead use the body def_id here?

bors · 2020-10-02T13:52:51Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ecstatic-morse · 2020-10-02T17:12:05Z

There's a follow-up to #77427, which is to remove the DefId from all (DefId, Body) pairs in function signatures. I'm not going to it as part of this PR (though I think I did a few functions by accident in pretty.rs) since it's already prone to merge conflicts.

ecstatic-morse · 2020-10-02T20:36:41Z

compiler/rustc_mir_build/src/build/mod.rs

-    tcx.alloc_steal_mir(mir_build(tcx, def))
+    let mut body = mir_build(tcx, def);
+    if def.const_param_did.is_some() {
+        assert!(matches!(body.source.instance, ty::InstanceDef::Item(_)));
+        body.source = MirSource::from_instance(ty::InstanceDef::Item(def.to_global()));
+    }
+
+    tcx.alloc_steal_mir(body)


@lcnr what should this look like in your eyes? It seems like we should do this when the MIR is built instead, but that means calling try_upgrade outside of a query?

let body = mir_built(txc, def); tcx.alloc_steal_mir(body)

we already have def as an input to mir_built, so we should be able to use the correct source right from the start here.

That causes cycle errors during type-checking.

edit: Sorry, I see my force-push erased the run results for the version of this PR without this commit. I'll try to find the failing test.

why do you think that we should call try_upgrade here?

I meant inside rustc_mir_builder::Builder::build() or so.

yeah, we should never have to call try_upgrade inside of mir_built after the initial call at the start of the query. If you can get the failing state again I would gladly look into what's going wrong there

error[E0391]: cycle detected when type-checking `main` --> /home/mackendy/src/rust/rust/alt/src/test/ui/const-generics/slice-const-param-mismatch.rs:18:33 | LL | let _: ConstBytes<b"AAA"> = ConstBytes::<{&[0x41, 0x41, 0x41]}>; | ^^^^^^^^^^^^^ | note: ...which requires simplifying constant for the type system `main::{constant#9}`... --> /home/mackendy/src/rust/rust/alt/src/test/ui/const-generics/slice-const-param-mismatch.rs:18:46 | LL | let _: ConstBytes<b"AAA"> = ConstBytes::<{&[0x41, 0x41, 0x41]}>; | ^^^^^^^^^^^^^^^^^^^^^ note: ...which requires const-evaluating + checking `main::{constant#9}`... --> /home/mackendy/src/rust/rust/alt/src/test/ui/const-generics/slice-const-param-mismatch.rs:18:46 | LL | let _: ConstBytes<b"AAA"> = ConstBytes::<{&[0x41, 0x41, 0x41]}>; | ^^^^^^^^^^^^^^^^^^^^^ = note: ...which requires normalizing `main::{constant#9}::promoted[0]`... note: ...which requires resolving instance `main::{constant#9}`... --> /home/mackendy/src/rust/rust/alt/src/test/ui/const-generics/slice-const-param-mismatch.rs:18:46 | LL | let _: ConstBytes<b"AAA"> = ConstBytes::<{&[0x41, 0x41, 0x41]}>; | ^^^^^^^^^^^^^^^^^^^^^ note: ...which requires computing the optional const parameter of `main::{constant#9}`... --> /home/mackendy/src/rust/rust/alt/src/test/ui/const-generics/slice-const-param-mismatch.rs:18:46 | LL | let _: ConstBytes<b"AAA"> = ConstBytes::<{&[0x41, 0x41, 0x41]}>; | ^^^^^^^^^^^^^^^^^^^^^ = note: ...which again requires type-checking `main`, completing the cycle = note: cycle used when type-checking all item bodies error: aborting due to 4 previous errors Some errors have detailed explanations: E0308, E0391. For more information about an error, try `rustc --explain E0308`. ------------------------------------------ failures: [ui] ui/const-generics/slice-const-param-mismatch.rs#full [ui] ui/const-generics/slice-const-param.rs#full

Cycle along with the two failing tests. This is what happens with just the first commit.

= note: ...which requires normalizing `main::{constant#9}::promoted[0]`...

Is probably where this goes wrong, so we somehow are not using the full def for promoteds.

Not at home rn, will look at where exactly this goes wrong tomorrow.

So your solution is probably the easiest for now...

The actual mir builder currently only uses a DefId and has quite a few entry points.
After we merge this I am going to look into a good way to start with the correct WithOptConstParam in the build::Builder.
The way I expect to do this is a bit like the following but without using WithOptConstParam::unknown there, which is a bit more invasive than I would like in this PR: lcnr@9061538

Apart from this I think this is mostly ready for merge 🤔 is there still something you want to do in that PR?

There's a cleaner way of doing this, but it involves passing `WithOptConstParam` around in more places. We're going to try to explore different approaches before committing to that.

ecstatic-morse · 2020-10-04T18:08:39Z

@bors p=1 rollup=never

(prone to merge conflicts)

@lcnr I cleaned up the commit messages and rebased. Nothing else has changed. This should be ready for the queue now.

lcnr · 2020-10-04T18:15:35Z

yeah, let's fix the remaining nits in followup PRs

@bors r+

bors · 2020-10-04T18:15:36Z

📌 Commit 606655e has been approved by lcnr

bors · 2020-10-04T18:24:08Z

⌛ Testing commit 606655e with merge 4ccf5f7...

bors · 2020-10-04T20:33:13Z

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 4ccf5f7 to master...

RalfJung · 2020-10-05T08:20:11Z

This PR introduced an ICE for mir-opt-level 2 and above: #77564.

lcnr · 2020-10-05T08:43:00Z

compiler/rustc_mir/src/transform/inline.rs

@@ -300,7 +301,7 @@ impl Inliner<'tcx> {

        // FIXME: Give a bonus to functions with only a single caller

-        let param_env = tcx.param_env(self.source.def_id());
+        let param_env = tcx.param_env(callee_body.source.def_id());


@ecstatic-morse this is the wrong source, will open a PR for this

opened #77568

Replace `(Body, DefId)` with `Body` where possible Follow-up to rust-lang#77430. I `grep`-ed for parameter lists in which a `Body` appeared within a few lines of a `DefId`, so it's possible that I missed some cases, but this should be pretty complete. Most of these changes were mechanical, but there's a few places where I started calling things "caller" and "callee" when multiple `DefId`s were in-scope at once. Also, we should probably have a helper function on `Body` that returns a `LocalDefId`. I can do that in this PR or in a follow-up.

@ecstatic-morse

…morse inliner: use caller param_env We used the callee param env instead of the caller param env by accident in rust-lang#77430, this PR fixes that and caches it in the `Inliner` struct. fixes rust-lang#77564 r? @ecstatic-morse

ecstatic-morse force-pushed the mir-source-in-body branch from b933634 to 8bc9f3d Compare October 1, 2020 22:23

ecstatic-morse force-pushed the mir-source-in-body branch from 8bc9f3d to 3510421 Compare October 2, 2020 01:30

rust-highfive assigned lcnr Oct 2, 2020

lcnr reviewed Oct 2, 2020

View reviewed changes

lcnr approved these changes Oct 2, 2020

View reviewed changes

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 2, 2020

ecstatic-morse force-pushed the mir-source-in-body branch from 920fa8f to ef49daa Compare October 2, 2020 17:14

ecstatic-morse commented Oct 2, 2020

View reviewed changes

ecstatic-morse added 4 commits October 4, 2020 10:59

Miscellaneous import formatting

141a977

Move MirSource to rustc_middle

938d8fd

Remember the MirSource for each Body

6f61e71

HACK: Overwrite the MIR's source with the correct const param

606655e

There's a cleaner way of doing this, but it involves passing `WithOptConstParam` around in more places. We're going to try to explore different approaches before committing to that.

ecstatic-morse force-pushed the mir-source-in-body branch from 187acab to 606655e Compare October 4, 2020 18:05

ecstatic-morse marked this pull request as ready for review October 4, 2020 18:05

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2020

bors merged commit 4ccf5f7 into rust-lang:master Oct 4, 2020

bors mentioned this pull request Oct 4, 2020

Prevent query cycles in the MIR inliner #68828

Merged

rustbot added this to the 1.49.0 milestone Oct 4, 2020

bors mentioned this pull request Oct 4, 2020

Collect statistics about MIR optimizations #76769

Closed

This was referenced Oct 4, 2020

Working branch-level code coverage #77080

Merged

New mir-opt pass to simplify gotos with const values #77486

Closed

ecstatic-morse mentioned this pull request Oct 4, 2020

Replace (Body, DefId) with Body where possible #77552

Merged

ecstatic-morse deleted the mir-source-in-body branch October 5, 2020 04:06

RalfJung mentioned this pull request Oct 5, 2020

[ICE] in normalize_erasing_regions with mir-opt-level=2 #77564

Closed

lcnr reviewed Oct 5, 2020

View reviewed changes

lcnr mentioned this pull request Oct 5, 2020

inliner: use caller param_env #77568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store a `MirSource` inside every `Body` #77430

Store a `MirSource` inside every `Body` #77430

ecstatic-morse commented Oct 1, 2020

ecstatic-morse commented Oct 1, 2020

ecstatic-morse commented Oct 2, 2020 •

edited

Loading

lcnr left a comment

lcnr Oct 2, 2020

ecstatic-morse Oct 2, 2020

lcnr Oct 2, 2020

ecstatic-morse Oct 2, 2020

lcnr Oct 4, 2020

ecstatic-morse Oct 4, 2020

lcnr Oct 2, 2020

ecstatic-morse Oct 2, 2020

lcnr Oct 2, 2020

lcnr Oct 2, 2020

lcnr Oct 2, 2020

bors commented Oct 2, 2020

ecstatic-morse commented Oct 2, 2020

ecstatic-morse Oct 2, 2020

lcnr Oct 2, 2020 •

edited

Loading

lcnr Oct 2, 2020

ecstatic-morse Oct 2, 2020 •

edited

Loading

lcnr Oct 2, 2020

lcnr Oct 2, 2020

ecstatic-morse Oct 2, 2020

lcnr Oct 2, 2020

lcnr Oct 4, 2020 •

edited

Loading

lcnr Oct 4, 2020

ecstatic-morse commented Oct 4, 2020

lcnr commented Oct 4, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

RalfJung commented Oct 5, 2020

lcnr Oct 5, 2020

lcnr Oct 5, 2020

	pub fn item(def_id: DefId) -> Self {
	pub fn item(def: ty::WithOptParam<DefId>) -> Self {

	add_moves_for_packed_drops(tcx, body, body.source.def_id());
	add_moves_for_packed_drops(tcx, body);

Store a MirSource inside every Body #77430

Store a MirSource inside every Body #77430

Conversation

ecstatic-morse commented Oct 1, 2020

ecstatic-morse commented Oct 1, 2020

ecstatic-morse commented Oct 2, 2020 • edited Loading

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 2, 2020

ecstatic-morse commented Oct 2, 2020

Choose a reason for hiding this comment

lcnr Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse commented Oct 4, 2020

lcnr commented Oct 4, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

RalfJung commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Store a `MirSource` inside every `Body` #77430

Store a `MirSource` inside every `Body` #77430

ecstatic-morse commented Oct 2, 2020 •

edited

Loading

lcnr Oct 2, 2020 •

edited

Loading

ecstatic-morse Oct 2, 2020 •

edited

Loading

lcnr Oct 4, 2020 •

edited

Loading