Stream: t-compiler/help

Topic: const args in type dependent paths


Bastian Kauschke (Apr 21 2020 at 20:35, on Zulip):

https://github.com/rust-lang/rust/pull/71154

Bastian Kauschke (Apr 21 2020 at 20:36, on Zulip):

Both incremental tests and cross crate builds don't fully work yet.

Bastian Kauschke (Apr 21 2020 at 20:49, on Zulip):

When calling typeck_tables_of(const_arg) where const_arg is from an external crate, we must have already computed and stored this.
As my PR calls typeck_tables_of_const_arg(const_arg, param_def_id) instead, this is not always the case.

I do not compute this twice for the local crate, as I call const_param_of at the start of typeck_tables_of. https://github.com/rust-lang/rust/blob/c94d6b5e1807b6487c676a75e4485f9becda8e2e/src/librustc_typeck/check/mod.rs#L981-L986
const_param_of also requires the HIR though.

I think that the easiest way to solve this is by storing const_param_of on disk for all const arguments and add a wrapper for typeck_tables_of(def_id) which calls const_param_of for all const arguments "inside" of def_idafter executing the query.
const_param_of can now safely call typeck_tables_of(body_owner) without cycles, as we just computed this.

We can now safely return None in const_param_of if tcx.hir().as_local_hir_id(def_id) fails. https://github.com/rust-lang/rust/blob/c94d6b5e1807b6487c676a75e4485f9becda8e2e/src/librustc_typeck/collect/type_of.rs#L24

Does this sound like a fairly clean approach to you? @varkor

Bastian Kauschke (Apr 22 2020 at 16:32, on Zulip):

Why is ensure used in typeck_item_bodies? https://github.com/rust-lang/rust/blob/00f677d8974b393ff32ca25bf916b6b9650c75b0/src/librustc_typeck/check/mod.rs#L751-L754

Bastian Kauschke (Apr 22 2020 at 17:49, on Zulip):

Are all const arguments body owners?

Bastian Kauschke (Apr 22 2020 at 20:47, on Zulip):

Incremental tests now pass on local, so I hope that most of my work is finally done here :laughing:

Thiis PR introduces a small amount of complexity and requires a lot of effort by @eddyb and @varkor to review it.
Are there some smallish + unrelated refactorings I can do in the near future?

@DPC if I remember correctly you had things you could use some help for

DPC (Apr 22 2020 at 20:47, on Zulip):

hi Bastian. Yeah i need to dig up some issues so i'll try today or tomorrow and ping you accordingly :slight_smile:

varkor (Apr 22 2020 at 20:52, on Zulip):

Sorry, I saw the notification, and then forgot to respond.

varkor (Apr 22 2020 at 20:52, on Zulip):

I'm not sure about ensure.

varkor (Apr 22 2020 at 20:54, on Zulip):

I'll try to review the PR reasonably soon.

varkor (Apr 22 2020 at 20:54, on Zulip):

But I have some deadlines this week, so I may not get to it right away.

Bastian Kauschke (Apr 23 2020 at 11:16, on Zulip):

https://github.com/rust-lang/rust/pull/71154 now passes CI, can someone run perf for this?

Bastian Kauschke (Apr 23 2020 at 13:38, on Zulip):
// run-pass
#![feature(const_generics)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash
#![feature(const_fn)]

struct Foo;

impl Foo {
    fn foo<const N: usize>(&self) -> usize {
        let f = self;
        f.bar::<{
            let f = Foo;
            f.bar::<7usize>()
        }>() + N
       //~^ how can I get the tyeck_tables of this block
    }

    const fn bar<const M: usize>(&self) -> usize {
        M
    }
}

fn main() {
    let f = Foo;

    assert_eq!(f.foo::<13>(), 0)
}

I currently use tcx.typeck_tables_of(tcx.hir().get_parent_did(tcx.hir().get_parent_node(hir_id)).to_def_id()) to get the DefId of

{
     let f = Foo;
     f.bar::<7usize>()
}

from 7usize. This doesn't work, as I get the fn foo instead

DPC (Apr 23 2020 at 13:51, on Zulip):

once github is working normally, sure :P

DPC (Apr 23 2020 at 13:53, on Zulip):

rdone

Bastian Kauschke (Apr 24 2020 at 17:58, on Zulip):

I changed https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/query/sealed/index.html to pub(crate) as I need to wrap some often used queries and want the wrapper to also accept both DefId andLocalDefId

Bastian Kauschke (Apr 24 2020 at 18:56, on Zulip):

@DPC can you retry the perf run :laughing: the last one failed for a reason I don't quite understand

DPC (Apr 24 2020 at 19:06, on Zulip):

sure

Bastian Kauschke (Apr 25 2020 at 07:39, on Zulip):

I thought that a rebase after @bors try does not stop perf :thinking: :sweat_smile:

Bastian Kauschke (Apr 25 2020 at 07:41, on Zulip):

@varkor I just realised how to simplify the PR :laughing: The longer it takes until you to have enough time, the easier it will be in the end.
Will take a few days to implement :sparkles:

Bastian Kauschke (Apr 25 2020 at 20:35, on Zulip):

Seems like it only took 1 day :laughing: https://github.com/rust-lang/rust/pull/71154#issuecomment-619437246

eddyb (Apr 27 2020 at 06:41, on Zulip):

I still won't get to it for a while, but maybe next weekend

Bastian Kauschke (Apr 27 2020 at 06:45, on Zulip):

@eddyb That's far sooner than I expected :heart: So don't worry

Bastian Kauschke (Apr 27 2020 at 09:36, on Zulip):

@DPC can you retry the perf run :laughing: the last one didn't run for a reason I don't quite understand

Bastian Kauschke (Apr 27 2020 at 09:37, on Zulip):

I won't touch this in the next few days, so we should be safe this time :laughing:

Bastian Kauschke (Apr 29 2020 at 10:07, on Zulip):

@eddybplease look at https://github.com/rust-lang/rust/pull/71477 beforehand. It's a tiny fix needed for https://github.com/rust-lang/rust/pull/71154

Bastian Kauschke (May 03 2020 at 10:41, on Zulip):

Once again thought of an improvement :laughing: pls no review rn

Bastian Kauschke (May 03 2020 at 13:14, on Zulip):

seems like this refactoring ended up being a dead end :disappointed: 124 files changed, 750 insertions(+), 800 deletions(-)

Bastian Kauschke (May 03 2020 at 21:07, on Zulip):

I am somewhat finished but am still not completely satisfied :oh_no: There are >40 calls to typeck_tables_of which I did not yet reach during typeck. As all I don't want to recompute typeck_tables_of twice for const arguments I added a thin wrapper which gets the correct DefId of the generic parameter and then calls the actual query(which is now renamed to _typeck_tables_of).

Bastian Kauschke (May 03 2020 at 21:08, on Zulip):

I would have to add the same wrapper to TyCtxtEnsure as there are multiple tcx.ensure().typeck_tables_of calls which I also had to manually edit.

Bastian Kauschke (May 03 2020 at 21:09, on Zulip):

I currently have 3 ways I could go:

Bastian Kauschke (May 03 2020 at 21:11, on Zulip):

remove the wrapper for typeck_tables_of and get the correct generic param at the start of this query and then call the same query with the correct arguments. This still requires me to modify all 40+ callsites of this function and causes cycle errors to be longer.

Bastian Kauschke (May 03 2020 at 21:14, on Zulip):

Change all occurances of typeck_tables_of(input) to typeck_tables_of(tcx.with_opt_param(input)). While this changes even more locations,
it doe s not require any wrappers and is probably the cleanest solution.

Bastian Kauschke (May 03 2020 at 21:16, on Zulip):

Also add a wrapper to TyCtxtEnsure for typeck_tables_of, this duplicates the wrapper and would be the first ever methods added to this struct, so I don't feel comfortable doing this. Also leaves us with _query_name, which I kind of dislike.

Bastian Kauschke (May 03 2020 at 21:17, on Zulip):

I am personally in favor of option 2 but dislike all of them :distraught:

Bastian Kauschke (May 04 2020 at 09:38, on Zulip):

@varkor The refactoring has been finished. Reduced the size to 101 files changed, 1069 insertions(+), 467 deletions(-) :sparkles:

lcnr (May 24 2020 at 22:35, on Zulip):

@varkor are you able to spend some time on #71154 this week? I think I am now fairly close to the optimal solution and considering its impact, I do want to get this done soon.

varkor (May 24 2020 at 23:01, on Zulip):

I will try to take a look soon, but I think @eddyb still wants to review as well before it is merged.

lcnr (Jun 05 2020 at 22:44, on Zulip):

Considering that MCPs are a thing now, this PR probably needs one :sweat_smile:

I have an initial draft at https://gist.github.com/lcnr/f977d902d3f509d20d69d0110dba2f3f
I am not sure on what I relevant here and what may need a more detailed explanation, so please ask for clarification or improvements where needed.

lcnr (Jun 05 2020 at 22:48, on Zulip):

I also talked a bit with eddyb about the current implementation and may still end up changing how exactly parameters are passed into queries, which hopefully isn't too relevant for this MCP.

lcnr (Jun 08 2020 at 12:18, on Zulip):

Opened https://github.com/rust-lang/compiler-team/issues/304

lcnr (Jun 08 2020 at 12:21, on Zulip):

@eddyb @varkor this PR still causes an ~1.5 % perf regression in some benchmarks

lcnr (Jun 08 2020 at 12:23, on Zulip):

This is most probably caused by calling tcx.with_opt_param(def_id) before calling an already executed query, meaning that we now have 2 hashtable lookups instead of one.

lcnr (Jun 08 2020 at 12:25, on Zulip):

A solutation suggested by eddyb and a previous approach was to instead allow queries to be called without the relevant param def id.
And only call tcx.const_param_of inside of the query:

fn unsafety_check_result(
    tcx: TyCtxt<'_>,
    def: ty::WithOptParam<LocalDefId>,
) -> UnsafetyCheckResult {
    if def.param_did.is_none() {
        if let Some(param_did) = tcx.const_param_of(def.did) {
            return tcx.unsafety_check_result(ty::WithOptParam {
                did: def.did,
                param_did: Some(param_did),
            });
        }
    }
    // ...
}
lcnr (Jun 08 2020 at 12:29, on Zulip):

Just found a solution to my problem :sweat_smile: Trying to explain exactly what's wrong works magic

lcnr (Jun 08 2020 at 12:31, on Zulip):

The problem was that we have to be able to reconstruct WithOptParam from a DepNode

lcnr (Jun 08 2020 at 12:32, on Zulip):

I previously did this by using dep_node.extract_def_id(tcx).map(|def_id| tcx.with_opt_param(def_id))

lcnr (Jun 08 2020 at 12:33, on Zulip):

This is wrong for WithOptParams where param_did != tcx.with_opt_param(def_id), resulting in ICE.

lcnr (Jun 08 2020 at 12:34, on Zulip):

Can be solved by splitting relevant queries into query_name(def_id) and query_name_for_const_arg(with_opt_param) and keeping the previous invariant of WithOptParam.

lcnr (Jun 08 2020 at 13:24, on Zulip):

Is there a quick way to profile how often a given query is called, even if it is already cached?

lcnr (Jun 08 2020 at 15:19, on Zulip):

Changed resolve_instance for now, which seems like it is the most frequently used query... Can someone start a perf run for https://github.com/rust-lang/rust/pull/71154?

lcnr (Jun 11 2020 at 07:13, on Zulip):

Well, I don't really know how what else I can try to fix perf here :disappointed: (without causing unsoundness)

The relevant perf results are https://perf.rust-lang.org/compare.html?start=e4124750c33631042d5f078b78ce16286b8027c0&end=384f9134ec3c49bfb8b7d20d1eaa9552358fc42b

lcnr (Jun 11 2020 at 07:20, on Zulip):

IMO this change is required to stabilize const generics and I am kind of unsure on how else we might implement this, so I think if noone else has an idea we should merge this even if it worsens perf by ~1.5% in some benchmarks

lcnr (Jun 11 2020 at 07:26, on Zulip):

I am very much biased towards getting const generics forward and don't care as much about perf, so there are probably other people who do want to see this merged, considering that const generics are still fairly far from stable.

lcnr (Jun 11 2020 at 07:27, on Zulip):

@varkor @eddyb What's your stance here?

lcnr (Jun 11 2020 at 07:33, on Zulip):

An easy fix would be to close #72962, merge its commits on top of #71154 and pretend like that PR is now perf neutral :upside_down:

lcnr (Jun 11 2020 at 09:07, on Zulip):

lcnr said:

I am very much biased towards getting const generics forward and don't care as much about perf, so there are probably other people who do want to see this merged, considering that const generics are still fairly far from stable.

other people who do not want to see this merged

varkor (Jun 15 2020 at 11:48, on Zulip):

@lcnr: sorry, I've been much busier than I expected these last couple of weeks, so I haven't managed to get to this PR, or really think about anything.

varkor (Jun 15 2020 at 12:16, on Zulip):

I'm personally in favour of accepting a regression like that, considering that it is necessary to move CG forward. But I may be slightly biased :)

varkor (Jun 15 2020 at 12:16, on Zulip):

I've had a read through the changes, and I don't see anything I think should be changed with the approach: my only real comments are about adding a few extra comments.

varkor (Jun 15 2020 at 12:17, on Zulip):

But @eddyb wanted to review this before any decision was made, so it's in their hands now.

lcnr (Jun 15 2020 at 13:27, on Zulip):

@varkor can I gently nudge you towards https://github.com/rust-lang/compiler-team/issues/304 :laughter_tears:

One weird quirk I am not completely sure about is https://github.com/rust-lang/rust/pull/71154/commits/03f17ccf7f9a77deac74a3abc17984e2710de8ea . This comment always uses None as param_did if the def_id is not local. Which should be fine as comparisons and hashing all only care about that field and we don't have to worry about cycles in these cases, but I want to explain this more clearly in a comment on WithOptParam itself, as it might be quite surprising

lcnr (Jun 15 2020 at 13:28, on Zulip):

Have an exam tomorrow so I will try to do it later this week

varkor (Jun 15 2020 at 15:27, on Zulip):

@lcnr: oh, what effect does this have on calling methods defined in other crates?

varkor (Jun 15 2020 at 15:27, on Zulip):

We should add a test for cross-crate calling, even if we don't expect it to be problematic, as we've overlooked issues there before.

lcnr (Jun 15 2020 at 15:33, on Zulip):

The benefit of this is that we don't have to do two seperate query lookups when calling methods from extern crates.
For the query cache WithOptParam { did, param_did: Some(param) } looks the same as WithOptParam { did, param_did: None }, so we still only cache it once. If something is reused computed both as local and extern, we may call type_of(arg) -> type_of(const_param_of(arg)) instead of type_of(const_param).

lcnr (Jun 15 2020 at 15:34, on Zulip):

for the local crate, each call of typeck_tables_of(def_id) first requires a call to const_param_of(def_id)

lcnr (Jun 15 2020 at 15:35, on Zulip):

I am not completely sold on this yete though, as the perf impact is fairly small afaict and its not very easy to reason about imo

varkor (Jun 15 2020 at 15:38, on Zulip):

Is there a perf run recording the difference?

varkor (Jun 15 2020 at 15:38, on Zulip):

lcnr said:

varkor can I gently nudge you towards https://github.com/rust-lang/compiler-team/issues/304 :laughter_tears:

Ah, sorry, I've been completely behind. I've just seconded it.

lcnr (Jun 15 2020 at 17:07, on Zulip):

With param_did: None: https://perf.rust-lang.org/compare.html?start=bb8674837a9cc5225020e07fc3f164762bb4c11c&end=0a204d6a1aef863bca037ca6e88ba49ce3e734ed

With param_did: tcx.const_param_of(def_id): https://perf.rust-lang.org/compare.html?start=e4124750c33631042d5f078b78ce16286b8027c0&end=384f9134ec3c49bfb8b7d20d1eaa9552358fc42b

lcnr (Jun 16 2020 at 19:01, on Zulip):

I tried to formulate why this is sound, but wasn't able to even remove my own doubts :grimacing:

Will revert that change, meh

lcnr (Jun 16 2020 at 20:15, on Zulip):

Can someone perf https://github.com/rust-lang/rust/pull/71154? Let's see where we are now

lqd (Jun 16 2020 at 20:26, on Zulip):

(it seems the run has been queued already -- however, for faster iteration times, you can also run the benchmarks locally https://github.com/rust-lang/rustc-perf/tree/master/collector#how-to-benchmark-a-change-on-your-own-machine)

lcnr (Jun 17 2020 at 08:01, on Zulip):

prrrrrrrrrrrf https://perf.rust-lang.org/compare.html?start=a647c0cd68bdd0f15081019f0b21bc31ae23f072&end=2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36

I guess this is what we end up with

lcnr (Jul 07 2020 at 13:25, on Zulip):

hmm, in https://github.com/rust-lang/rust/pull/74113 it seems like @bors try finished but nothing happened afterwards :oh_no:

Does someone know what happend there?

Matthew Jasper (Jul 07 2020 at 13:32, on Zulip):

If you push after @bors try then bors won't post a completed message. You can use @rustc-perf build <commit-hash> to run perf.

Last update: Sep 28 2020 at 15:45UTC