Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drive trans from the output of the translation item collector #33890

Merged
merged 21 commits into from
Jul 8, 2016

Conversation

michaelwoerister
Copy link
Member

This PR changes the way how translation works above the item level. Instead of walking the HIR and calling trans_item() on everything encountered (while instantiating monomorphizations on-demand), we now just process the list of translation items generated by the trans::collector. Using the collector has the benefit of being able to know the exact set of monomorphizations and symbols before actually running translation, something that is crucial for incremental compilation (but also has other benefits).

The collector has existed for quite a while now, but so far it's output was only used for running some auto-tests. With this PR it becomes the only source of truth about what gets translated.

One modification we had to make, compared to the initial approach, is that closures are not represented as their own TransItems. Doing so, while still supporting non-MIR-based translation, would have been prohibitively complex, and not worth the trouble since legacy-trans will disappear sooner or later. Once there is solely MIR-trans, it would be a good idea to make closures TransItems again.

This PR removes the most obvious functions and tables that are not needed anymore, but there's definitely still more cleanup possible later on (e.g. monomorphize::monomorphic_fn() does very little at this point). Since there are already more than 10 commits in here, doing this in a separate PR seems to be a better idea.

These changes definitely warrant a crater run.

Thanks @Aatch, for taking on one of the more tedious tasks during the dev-sprint!
Thanks @eddyb, for doing some nice refactorings to symbol name generation and making sure these landed so I could use them!

cc @rust-lang/compiler
cc @rust-lang/tools

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

}
}
}
}
hir::ItemEnum(ref enum_definition, ref gens) => {
if gens.ty_params.is_empty() {
// sizes only make sense for non-generic types
enum_variant_size_lint(ccx, enum_definition, item.span, item.id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be a regular lint using ty::layout now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preexisting, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, mostly making a note as it's the only thing I can see left in that function.

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

(or @eddyb ?)

@bors
Copy link
Contributor

bors commented May 26, 2016

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

@michaelwoerister michaelwoerister force-pushed the collector-driven-trans branch from f972fbb to 7e3b445 Compare May 27, 2016 14:27
@michaelwoerister
Copy link
Member Author

Rebased...

@bors
Copy link
Contributor

bors commented May 30, 2016

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

}) => {
attributes::from_fn_attrs(ccx, attrs, lldecl);

let is_first = !ccx.available_monomorphizations().borrow()
.contains(&symbol);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so...was this code just dead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this logic was moved elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the partitioner is now responsible for assigning the correct linkage to monomorphizations.

@nikomatsakis
Copy link
Contributor

OK, so, I gave this a once over and it all makes sense. I left various comments. r=me but I'd like @eddyb to sign off too.

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned nikomatsakis May 31, 2016
@nikomatsakis
Copy link
Contributor

Kicked off a crater run, but it occurs to me that a more likely source of failure is Windows and the like.

@eddyb
Copy link
Member

eddyb commented Jun 1, 2016

LGTM, other than @nikomatsakis' comments.

@nikomatsakis
Copy link
Contributor

@michaelwoerister crater run yielded 8 regressions. I looked at a few of them and they looked legit:

https://gist.github.com/nikomatsakis/4b1496d1b60e77cac2fe8c604a6ebec2

@michaelwoerister
Copy link
Member Author

Thanks for the crater run! Hopefully the regressions can be fixed quickly.

@michaelwoerister michaelwoerister force-pushed the collector-driven-trans branch from 7e3b445 to 91e9bda Compare June 2, 2016 15:32
@michaelwoerister
Copy link
Member Author

So, if been looking into the first regression from the crater run and it seems that MIR is sufficiently different from old-trans that it breaks. The culprit here is the following function:

fn exists<P: AsRef<Path>>(path: P) -> io::Result<bool> {
    match fs::metadata(path) {
        Ok(_) => Ok(true),
        Err(ref err) if err.kind() == io::ErrorKind::NotFound => Ok(false),
        Err(err) => Err(err)
    }
}

When compiling with -Zorbit everything is fine, but when compiling with old-trans there is a linker error because old-trans expects a monomorphization for exists<&&Path> but there is only one for exists<&PathBuf>. Does anyone have an idea how that could happen? Does lowering to MIR do something different with auto-deref maybe?

@jonas-schievink
Copy link
Contributor

@michaelwoerister This also seems to break a test when compiling with CGU (works fine without): https://gist.github.com/jonas-schievink/ad3fbdc66029a287f73ca9d2f3229027

(this is with make check-stage1-rpass RUSTFLAGS="-Ccodegen-units=4" TESTNAME="associated-types-normalize-unifield-struct")

@michaelwoerister michaelwoerister force-pushed the collector-driven-trans branch from 64a9f96 to 1c03bfe Compare July 8, 2016 14:44
@michaelwoerister
Copy link
Member Author

@bors r=eddyb

Re-added a missing use statement that had been removed in the meantime.

@bors
Copy link
Contributor

bors commented Jul 8, 2016

📌 Commit 1c03bfe has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 8, 2016

⌛ Testing commit 1c03bfe with merge d119362...

bors added a commit that referenced this pull request Jul 8, 2016
Drive trans from the output of the translation item collector

This PR changes the way how translation works above the item level. Instead of walking the HIR and calling `trans_item()` on everything encountered (while instantiating monomorphizations on-demand), we now just process the list of translation items generated by the `trans::collector`. Using the collector has the benefit of being able to know the exact set of monomorphizations and symbols before actually running translation, something that is crucial for incremental compilation (but also has [other benefits](#33602)).

The collector has existed for quite a while now, but so far it's output was only used for running some auto-tests. With this PR it becomes the only source of truth about what gets translated.

One modification we had to make, compared to the initial approach, is that closures are not represented as their own `TransItems`. Doing so, while still supporting non-MIR-based translation, would have been prohibitively complex, and not worth the trouble since legacy-trans will disappear sooner or later. Once there is solely MIR-trans, it would be a good idea to make closures `TransItems` again.

This PR removes the most obvious functions and tables that are not needed anymore, but there's definitely still more cleanup possible later on (e.g. `monomorphize::monomorphic_fn()` does very little at this point). Since there are already more than 10 commits in here, doing this in a separate PR seems to be a better idea.

These changes definitely warrant a crater run.

Thanks @Aatch, for taking on one of the more tedious tasks during the dev-sprint!
Thanks @eddyb, for doing some nice refactorings to symbol name generation and making sure these landed so I could use them!

cc @rust-lang/compiler
cc @rust-lang/tools
@bors bors merged commit 1c03bfe into rust-lang:master Jul 8, 2016
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 11, 2024
…s, r=compiler-errors

Fix codegen-units tests that were disabled 8 years ago

I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 11, 2024
…s, r=compiler-errors

Fix codegen-units tests that were disabled 8 years ago

I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 12, 2024
…s, r=compiler-errors

Fix codegen-units tests that were disabled 8 years ago

I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 12, 2024
…s, r=compiler-errors

Fix codegen-units tests that were disabled 8 years ago

I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2024
Rollup merge of rust-lang#128929 - saethlin:enable-codegen-units-tests, r=compiler-errors

Fix codegen-units tests that were disabled 8 years ago

I don't know if any of these tests still have value. They were disabled by rust-lang#33890, and we've survived without them for a while. But considering how small this test suite is, maybe it's worth having them.

I also had to add some normalization to the codegen-units tests output. I think the fact that I had to add some underscores how poor our test coverage is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.