incr.comp.: Turn translation-related attributes into a query #48712

wesleywiser · 2018-03-04T02:37:17Z

bors · 2018-03-04T07:14:15Z

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

michaelwoerister

Thanks a lot for the very thorough PR, @wesleywiser! This makes a lot of things much nicer, I think, and probably more efficient too.

I added some comments where things need addressing. Just minor things though.

One thing left to do is caching the new query. Doing so is a bit more involved than it should be but it's pretty simple nonetheless:

Mark the query as cacheable like here:

rust/src/librustc/ty/maps/config.rs

Line 685 in e2746d8

impl_disk_cacheable_query!(def_symbol_name, |_| true);
Allow for the query to be "refreshed" before writing back out to disk:

rust/src/librustc/ty/maps/plumbing.rs

Line 999 in e2746d8

ContainsExternIndicator => contains_extern_indicator,
Actually write query results to disk, like here:

rust/src/librustc/ty/maps/on_disk_cache.rs

Line 220 in e2746d8

encode_query_results::<contains_extern_indicator, _>(tcx, enc, qri)?;

michaelwoerister · 2018-03-05T12:50:40Z

src/librustc_trans/attributes.rs

+    } else if trans_fn_attrs.flags.contains(TransFnAttrFlags::UNWIND) {
+        unwind(llfn, true);
+    } else if trans_fn_attrs.flags.contains(TransFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) {
+        unwind(llfn, false);


This transformation isn't quite right: We need to always go through all of the ifs instead of chaining them via else. The previous version used else because the body of the for loop would be executed for each attribute individually.

michaelwoerister · 2018-03-05T12:56:03Z

src/librustc/ich/impls_hir.rs

@@ -1156,6 +1158,14 @@ impl<'hir> HashStable<StableHashingContext<'hir>> for hir::TransFnAttrFlags
    }
 }

+impl<'hir> HashStable<StableHashingContext<'hir>> for attr::InlineAttr {


You should be able to use the impl_stable_hash_for! macro, like here:

rust/src/librustc/ich/impls_hir.rs

Lines 144 to 149 in e2746d8

impl_stable_hash_for!(enum hir::LifetimeName {

Implicit,

Underscore,

Static,

Name(name)

});

I tried that but the macro doesn't seem to like the path to that enum. Trying impl_stable_hash_for!(enum attr::InlineAttr { ... }) yields error[E0433]: failed to resolve. Did you mean syntax::attr?

but changing that to impl_stable_hash_for!(enum syntax::attr::InlineAttr { ... }) yields error[E0433]: failed to resolve. Use of undeclared type or module syntax. I'm not sure where to go from here...

michaelwoerister · 2018-03-05T12:59:36Z

src/librustc/ich/impls_hir.rs

@@ -1144,6 +1145,7 @@ impl<'hir> HashStable<StableHashingContext<'hir>> for hir::TransFnAttrs
                                          hcx: &mut StableHashingContext<'hir>,
                                          hasher: &mut StableHasher<W>) {
        self.flags.hash_stable(hcx, hasher);
+        self.inline.hash_stable(hcx, hasher);


We usually de-structure the data types in these implementations:

fn hash_stable<W: StableHasherResult>(&self, hcx: &mut StableHashingContext<'hir>, hasher: &mut StableHasher<W>) { let hir::TransFnAttrs { flags, inline, } = *self; flags.hash_stable(hcx, hasher); inline.hash_stable(hcx, hasher); }

This way the code contains an exhaustive list of all fields and one cannot add or remove a field without also updating the impl here. It's a bit more verbose but it is a good way of letting the compiler help keeping things consistent.

michaelwoerister · 2018-03-05T13:05:09Z

src/librustc_mir/transform/inline.rs

@@ -207,7 +207,7 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
        }

        let attrs = tcx.get_attrs(callsite.callee);


We should be able to get rid of this attrs entirely.

Just saw that this is fixed in a later commit :)

michaelwoerister · 2018-03-05T13:21:02Z

src/librustc_trans_utils/trans_crate.rs

-            Lrc::new(Vec::new()) // Just a dummy
+
+        providers.target_features_whitelist = |_tcx, _cnum| {
+            Rc::new(FxHashSet()) // Just a dummy


This will probably need updating to Lrc too.

bors · 2018-03-05T21:47:39Z

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

wesleywiser · 2018-03-06T03:04:57Z

@michaelwoerister I resolved all of your feedback except for the issue I mentioned above with the impl_stable_hash_for!(enum) macro.

michaelwoerister · 2018-03-06T08:35:45Z

Yes, the macro is sometimes weird like that. With an absolute path (starting with ::syntax) it would probably work.

Thanks for fixing the nits! This looks great!

@bors try (I'd like to see if this affects performance)

@bors r+

bors · 2018-03-06T08:35:46Z

📌 Commit fbcdef5 has been approved by michaelwoerister

bors · 2018-03-06T08:35:56Z

⌛ Trying commit fbcdef582ec8be6eeef06d90463b226166d07d22 with merge b773dbdc99823e59733116d290342079ba1ca26e...

bors · 2018-03-06T10:41:40Z

☀️ Test successful - status-travis
State: approved=michaelwoerister try=True

bors · 2018-03-06T10:41:49Z

⌛ Testing commit fbcdef582ec8be6eeef06d90463b226166d07d22 with merge f24577a22bd6f638589992267b87d23fde912871...

michaelwoerister · 2018-03-06T10:44:32Z

@bors r-

michaelwoerister · 2018-03-06T10:50:26Z

I seems that bors has decided to push this PR to the front of the queue (and kill the one that was almost finished :(). If it goes through, we don't need a separate perf run. Note to myself: don't r+ and try at the same time...

michaelwoerister · 2018-03-06T11:12:35Z

@bors r+ (so that bors doesn't suddenly decide to not merge even though all tests have passed)

bors · 2018-03-06T11:12:36Z

📌 Commit fbcdef5 has been approved by michaelwoerister

michaelwoerister · 2018-03-06T13:26:29Z

@Mark-Simulacrum, could you do a perf run please?

bors · 2018-03-06T15:04:48Z

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

Mark-Simulacrum · 2018-03-06T18:17:24Z

Perf started for b773dbdc99823e59733116d290342079ba1ca26e.

michaelwoerister · 2018-03-06T20:03:40Z

Thanks, @Mark-Simulacrum!

Here's the link: http://perf.rust-lang.org/compare.html?start=1733a61141d125beb45587dd89d54cd4a01cdd5a&end=b773dbdc99823e59733116d290342079ba1ca26e&stat=instructions%3Au

Will this work? Are there build artifacts for the commit? The try-build was f24577a.

Part of rust-lang#47320

wesleywiser · 2018-03-07T01:37:54Z

Rebased

Mark-Simulacrum · 2018-03-07T02:17:46Z

There were two try builds, of which bors seems to think only one completed so I ran perf for that one. The URL you provided looks correct.

michaelwoerister · 2018-03-07T08:59:23Z

@bors r+ (after rebase)

bors · 2018-03-07T08:59:24Z

📌 Commit e0f7527 has been approved by michaelwoerister

michaelwoerister · 2018-03-07T09:04:08Z

Thanks, @Mark-Simulacrum, yes the link works now, it's all good :)

The numbers look good too! Pretty much in line with what I expected.

wesleywiser · 2018-03-07T14:56:10Z

@michaelwoerister Are you concerned about the style-servo and style-servo-opt max regressions?

michaelwoerister · 2018-03-07T15:24:14Z

No, that's fine, the clean-incremental benchmark for style-servo is not stable: #48184
There's something in there that causes a varying number of CGUs to be recompiled even if there is no change.

wesleywiser · 2018-03-07T15:27:25Z

Got it. Thanks!

rust-highfive assigned michaelwoerister Mar 4, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2018

wesleywiser force-pushed the incr_attr_queries branch from 8824ae1 to 7327c91 Compare March 4, 2018 22:15

michaelwoerister reviewed Mar 5, 2018

View reviewed changes

wesleywiser force-pushed the incr_attr_queries branch from 7327c91 to 628818c Compare March 6, 2018 03:03

wesleywiser force-pushed the incr_attr_queries branch from 628818c to fbcdef5 Compare March 6, 2018 03:21

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 Mar 6, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 6, 2018

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 Mar 6, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 6, 2018

wesleywiser added 9 commits March 6, 2018 19:58

Add query for trans fn attributes

e8cd6cc

Part of rust-lang#47320

Add inline to TransFnAttrs

4f840a6

Part of rust-lang#47320

Replace attr::contains_name(..., "cold")

97b30f0

Part of rust-lang#47320

Remove export_name query

5460b88

Part of rust-lang#47320

Remove the contains_extern_indicator query

6bc7f41

Part of rust-lang#47320

Add flag for rustc_std_internal_symbol attribute

39f9d23

Part of rust-lang#47320

Add target_features to TransFnAttrs

07890c5

Part of rust-lang#47320

Add linkage to TransFnAttrs

368e586

Part of rust-lang#47320

Add caching for trans_fn_attrs query

e0f7527

Part of rust-lang#47320

wesleywiser force-pushed the incr_attr_queries branch from fbcdef5 to e0f7527 Compare March 7, 2018 01:36

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 Mar 7, 2018

alexcrichton merged commit e0f7527 into rust-lang:master Mar 8, 2018

wesleywiser mentioned this pull request Mar 13, 2018

Querify WF-checking so it can be cached #48939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incr.comp.: Turn translation-related attributes into a query #48712

incr.comp.: Turn translation-related attributes into a query #48712

wesleywiser commented Mar 4, 2018

bors commented Mar 4, 2018

michaelwoerister left a comment

michaelwoerister Mar 5, 2018

michaelwoerister Mar 5, 2018

wesleywiser Mar 6, 2018

michaelwoerister Mar 5, 2018

michaelwoerister Mar 5, 2018

michaelwoerister Mar 5, 2018

michaelwoerister Mar 5, 2018

bors commented Mar 5, 2018

wesleywiser commented Mar 6, 2018 •

edited

Loading

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

Mark-Simulacrum commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

wesleywiser commented Mar 7, 2018

Mark-Simulacrum commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

bors commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

wesleywiser commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

wesleywiser commented Mar 7, 2018

	impl_stable_hash_for!(enum hir::LifetimeName {
	Implicit,
	Underscore,
	Static,
	Name(name)
	});

		@@ -207,7 +207,7 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
		}

		let attrs = tcx.get_attrs(callsite.callee);

incr.comp.: Turn translation-related attributes into a query #48712

incr.comp.: Turn translation-related attributes into a query #48712

Conversation

wesleywiser commented Mar 4, 2018

bors commented Mar 4, 2018

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

wesleywiser Mar 6, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 5, 2018

Choose a reason for hiding this comment

bors commented Mar 5, 2018

wesleywiser commented Mar 6, 2018 • edited Loading

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

bors commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

bors commented Mar 6, 2018

Mark-Simulacrum commented Mar 6, 2018

michaelwoerister commented Mar 6, 2018

wesleywiser commented Mar 7, 2018

Mark-Simulacrum commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

bors commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

wesleywiser commented Mar 7, 2018

michaelwoerister commented Mar 7, 2018

wesleywiser commented Mar 7, 2018

wesleywiser commented Mar 6, 2018 •

edited

Loading