Stream: t-compiler

Topic: why do we invoke the collector twice?


nikomatsakis (Dec 26 2018 at 19:26, on Zulip):

So @eddyb, @mw, or somebody, perhaps one of you can explain this code to me:

fn codegen_crate<'a, 'tcx>(
        &self,
        tcx: TyCtxt<'a, 'tcx, 'tcx>,
        _rx: mpsc::Receiver<Box<dyn Any + Send>>
) -> Box<dyn Any> {
    ...
    ::rustc_mir::monomorphize::assert_symbols_are_distinct(tcx,
            collector::collect_crate_mono_items(
                tcx,
                collector::MonoItemCollectionMode::Eager
            ).0.iter()
    ...
    for mono_item in
            collector::collect_crate_mono_items(
                tcx,
                collector::MonoItemCollectionMode::Eager
            ).0 {
            ...
    }
}

unless i'm missing something, won't this run the collector twice?

nikomatsakis (Dec 26 2018 at 19:27, on Zulip):

I would expect a query or something

nikomatsakis (Dec 26 2018 at 19:27, on Zulip):

maybe that code is dead or something? (not obviously true)

mw (Jan 07 2019 at 09:50, on Zulip):

Yes, that just looks wrong. It's probably meant to call the collect_and_partition_mono_items query.

nikomatsakis (Jan 07 2019 at 15:50, on Zulip):

should we file an issue @mw ?

mw (Jan 07 2019 at 15:58, on Zulip):

yes

nikomatsakis (Jan 07 2019 at 16:01, on Zulip):

@mw filed https://github.com/rust-lang/rust/issues/57406

mw (Jan 07 2019 at 16:26, on Zulip):

:+1:

lqd (Jan 07 2019 at 19:19, on Zulip):

are there specific tests to run for this issue apart from the codegen ones ? mostly I'm wondering if changing the collector mode from eager to "whatever the query is doing" (that is, lazy unless we asked to link dead code) will need special care ?

lqd (Jan 07 2019 at 19:20, on Zulip):

(I guess I can also open a WIP PR and see what travis complains about)

lqd (Jan 07 2019 at 19:46, on Zulip):

I guess I can also open a WIP PR

#57418
(and thanks @nagisa I wondered whether I should have mentioned in the PR description the reason for removing the assert_symbols_are_distinct)

nagisa (Jan 07 2019 at 20:00, on Zulip):

Never hurts to mention it, so that a reviewer doesn’t need to go and verify :slight_smile:

lqd (Jan 08 2019 at 15:14, on Zulip):

turns out this is an unimportant codegen backend not the LLVM one, some comments might have saved us all the confusion :)

nikomatsakis (Jan 08 2019 at 16:06, on Zulip):

I realized that already, still seems bad

nikomatsakis (Jan 08 2019 at 16:06, on Zulip):

but if I thought it was on the main code path I would have made a bigger deal of it :)

lqd (Jan 08 2019 at 16:13, on Zulip):

I wonder if there are tests for this specific code path

lqd (Jan 08 2019 at 18:17, on Zulip):

(apparently so, nice)

lqd (Jan 09 2019 at 09:40, on Zulip):

@mw what do you want me to do for #57418 ? I'm not sure how to test this backend (it seems to be used in one run-make-fulldeps test) to make sure the behaviour doesn't change (and bjorn3 thought it was broken). I was thinking: 1) remove the inst.def.is_inline(tcx) calls you mentioned 2) add a comment explaining what this backend does (apparently it's part of a bigger picture to allow rustc to be built without LLVM)

mw (Jan 09 2019 at 11:03, on Zulip):

I'm not sure what this backend is used for, either. I think even the RLS needs LLVM at the moment.

mw (Jan 09 2019 at 11:04, on Zulip):

I'd say, if the run-make-fulldeps tests pass, it should be fine.

mw (Jan 09 2019 at 11:04, on Zulip):

I'd also start with removing the is_inline calls and see if that breaks any tests.

lqd (Jan 09 2019 at 13:08, on Zulip):

hmm bjorn3 says this backend is not really used

mw (Jan 09 2019 at 13:09, on Zulip):

OK, in that case, anything that doesn't break tests is fine, I'd say.

lqd (Jan 09 2019 at 13:15, on Zulip):

I've removed the is_inline call, and it doesn't change the run-make-fulldeps tests locally; I'll push the commit so that we can check Travis (update: commit pushed)

Last update: Nov 22 2019 at 04:55UTC