[WIP] improve canonicalization by using a hashing scheme #51837

JaimeValdemoros · 2018-06-27T08:26:20Z

Attempting at #48417. This branch is a little bit behind so I think the first thing I need to do is rebase onto current master.

rust-highfive · 2018-06-27T08:26:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

michaelwoerister · 2018-06-27T15:04:01Z

r? @nikomatsakis

nikomatsakis

+1

nikomatsakis · 2018-06-27T18:47:48Z

So this all seems good — I'm trying to remember what I had in mind. I know I considered going down this route (of adding hash_with to TypeFoldable).

There is also, however, a pre-existing "stable hash" thing we use for incremental. It may be that we can re-use some of that?

But I guess we need to be able to have some kind of "hook" (precisely what this approach provides us), so that when we are hashing in the canonicalization code, we can identify that we're looking at an unbound inference variable and substitute the hash for the next index, etc.

nikomatsakis · 2018-06-27T18:48:43Z

src/librustc/mir/mod.rs

+
+    fn super_hash_with<H: TypeHasher<'tcx>>(&self, _hasher: &mut H) -> u64 {
+        unimplemented!()
+    }
 }

 impl<'tcx> TypeFoldable<'tcx> for Operand<'tcx> {


I wonder if these impls can be moved to the macros -- I moved a bunch of stuff in preparation for this PR, but not everything. Trying to move as many things we can to use the macro -- before we make any changes to it! -- might indeed be a good first step.

Ok, I've made a start on this - I've picked out a couple that seem to be trivially applicable to the macros. For a couple of the types, it seems that only some of the fields (in the case of structs) and components of variants (in the case of enums) are super_folded or super_visited. Would an approach be to somehow 'tag' these in the macro and extend the macro to understand the difference?

I'm imagining for example the impl for RValue, which does something like:

match *self { Use(ref op) => Use(op.fold_with(folder)), Repeat(ref op, len) => Repeat(op.fold_with(folder), len), Ref(region, bk, ref place) => Ref(region.fold_with(folder), bk, place.fold_with(folder)), Len(ref place) => Len(place.fold_with(folder)), *snip* match *self { Use(ref op) => op.visit_with(visitor), Repeat(ref op, _) => op.visit_with(visitor), Ref(region, _, ref place) => region.visit_with(visitor) || place.visit_with(visitor), Len(ref place) => place.visit_with(visitor),

and the macro could be something like:

EnumTypeFoldableImpl! { impl<'tcx, T> TypeFoldable<'tcx> for Rvalue<'tcx> { (Rvalue::Use)(op), (Rvalue::Repeat)(op, @clone len), (Rvalue::Ref)(region, @clone bk, place), (Rvalue::Len)(place), ... } }

No, I've just implemented fold for those types that need it (e.g., bk) -- many of them already have it, I suspect.

nikomatsakis · 2018-06-27T18:50:13Z

src/librustc/ty/structural_impls.rs

+    fn super_hash_with<H: TypeHasher<'tcx>>(&self, hasher: &mut H) -> u64 {
+        use ty::InstanceDef::*;
+        self.substs.hash_with(hasher);
+        match self.def {


these hasher impls don't quite look right. We need to hash mem::discriminant(self) as well. Here is a good example:

rust/src/librustc/ich/impls_mir.rs

Lines 38 to 54 in ed0350e

impl<'a> HashStable<StableHashingContext<'a>>

for mir::BorrowKind {

#[inline]

fn hash_stable<W: StableHasherResult>(&self,

hcx: &mut StableHashingContext<'a>,

hasher: &mut StableHasher<W>) {

mem::discriminant(self).hash_stable(hcx, hasher);

match *self {

mir::BorrowKind::Shared |

mir::BorrowKind::Unique => {}

mir::BorrowKind::Mut { allow_two_phase_borrow } => {

allow_two_phase_borrow.hash_stable(hcx, hasher);

}

}

}

}

Ah, got it - was worrying about the fact that I was hashing a 'flattened' structure rather than the tree structure but I wasn't entirely sure how to resolve it. Presumably this is effectively adding in the enum 'position' to the hash

rust-highfive · 2018-06-29T08:49:50Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:53] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:54] tidy error: /checkout/src/librustc/ty/structural_impls.rs:995: line longer than 100 chars
[00:04:54] tidy error: /checkout/src/librustc/ty/structural_impls.rs:1020: line longer than 100 chars
[00:04:55] some tidy checks failed
[00:04:55] 
[00:04:55] 
[00:04:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:55] 
[00:04:55] 
[00:04:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:55] Build completed unsuccessfully in 0:01:58
[00:04:55] Build completed unsuccessfully in 0:01:58
[00:04:55] Makefile:79: recipe for target 'tidy' failed
[00:04:55] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09a0058d
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:14d8ef80:start=1530262108446766476,finish=1530262108452934981,duration=6168505
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1a7ab24b
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2e939b2c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2018-06-29T10:06:36Z

@JaimeValdemoros I'm starting to get worried that this will be a harder task than I originally imagined. =) I'd love to see if we can carve it up so that we can split the work a bit (this is why I was pinging you and @technicalguy about dropping in on zulip at some point over in #48417 -- but maybe we can chat a bit on the PR instead).

The main thing I can see right now is to start by having someone try to do all the rote work of extracting and applying the macros. I could take a stab at that, for sure, or you — but it seems like work that can land independently.

I suppose it may actually just be faster to do the hash_with impls manually -- it seems like you've already done a lot. I think the last time I was modifying TypeFoldable I wound up converting a ton of stuff to use macros but at some point I was like "screw it, I'm doing it with copy-n-paste from here". I guess I should check how many more super_hash_with fns you are missing =)

stokhos · 2018-07-06T19:27:07Z

Ping from triage @JaimeValdemoros, we haven't heard from you for a while, will you have time to look into this PR?

XAMPPRocky · 2018-07-16T18:45:37Z

Triage: @JaimeValdemoros I'm closing this as there's been no activity for the past 14 days. Thank you for this work and please do reopen this when you have time to come back to it.

rust-highfive assigned michaelwoerister Jun 27, 2018

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

JaimeValdemoros mentioned this pull request Jun 27, 2018

🛠 improve canonicalization by using a hashing scheme #48417

Closed

rust-highfive assigned nikomatsakis and unassigned michaelwoerister Jun 27, 2018

nikomatsakis reviewed Jun 27, 2018

View reviewed changes

nikomatsakis requested changes Jun 27, 2018

View reviewed changes

JaimeValdemoros added 4 commits June 29, 2018 09:23

Add hash_with method to TypeFoldable and define TypeHasher trait

b46258f

Implement a bunch of super_hash_with

0c618ab

Use the TypeFoldable macros for Operand and Field

5e302e8

Hash the discriminant in super_hash_with for enums

d4c414f

stokhos 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 Jul 6, 2018

XAMPPRocky closed this Jul 16, 2018

XAMPPRocky added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] improve canonicalization by using a hashing scheme #51837

[WIP] improve canonicalization by using a hashing scheme #51837

JaimeValdemoros commented Jun 27, 2018

rust-highfive commented Jun 27, 2018

michaelwoerister commented Jun 27, 2018

nikomatsakis left a comment

nikomatsakis commented Jun 27, 2018

nikomatsakis Jun 27, 2018

JaimeValdemoros Jun 29, 2018 •

edited

Loading

nikomatsakis Jun 29, 2018

nikomatsakis Jun 27, 2018

JaimeValdemoros Jun 29, 2018

nikomatsakis Jun 29, 2018

rust-highfive commented Jun 29, 2018

nikomatsakis commented Jun 29, 2018

stokhos commented Jul 6, 2018

XAMPPRocky commented Jul 16, 2018

	impl<'a> HashStable<StableHashingContext<'a>>
	for mir::BorrowKind {
	#[inline]
	fn hash_stable<W: StableHasherResult>(&self,
	hcx: &mut StableHashingContext<'a>,
	hasher: &mut StableHasher<W>) {
	mem::discriminant(self).hash_stable(hcx, hasher);

	match *self {
	mir::BorrowKind::Shared \|
	mir::BorrowKind::Unique => {}
	mir::BorrowKind::Mut { allow_two_phase_borrow } => {
	allow_two_phase_borrow.hash_stable(hcx, hasher);
	}
	}
	}
	}

[WIP] improve canonicalization by using a hashing scheme #51837

[WIP] improve canonicalization by using a hashing scheme #51837

Conversation

JaimeValdemoros commented Jun 27, 2018

rust-highfive commented Jun 27, 2018

michaelwoerister commented Jun 27, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 27, 2018

nikomatsakis Jun 27, 2018

Choose a reason for hiding this comment

JaimeValdemoros Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2018

Choose a reason for hiding this comment

nikomatsakis Jun 27, 2018

Choose a reason for hiding this comment

JaimeValdemoros Jun 29, 2018

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2018

Choose a reason for hiding this comment

rust-highfive commented Jun 29, 2018

nikomatsakis commented Jun 29, 2018

stokhos commented Jul 6, 2018

XAMPPRocky commented Jul 16, 2018

JaimeValdemoros Jun 29, 2018 •

edited

Loading