Add a packed/tagged pointer abstraction and utilize it for ParamEnv #75590

Mark-Simulacrum · 2020-08-16T15:12:04Z

The intent here is mostly just to add the abstraction; I suspect that there are definitely more use cases for it, and we can explore those over time now that there's a (mostly) safe abstraction that can be used in rustc.

rust-highfive · 2020-08-16T15:12:07Z

r? @ecstatic-morse

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

Mark-Simulacrum · 2020-08-16T15:15:54Z

I was hoping that we could let rustc take care of the packing for us with layout optimizations, but currently that's not quite possible: we need something like #[repr(no_reference, transparent)] or so which would permit rustc to use niches that prevent taking references to the inner field. I think adding something like that would be quite worthwhile, but I don't know how feasible that is. cc @eddyb

I am also unhappy with needing to split between Copy/Drop tagged pointers but AFAICT that's not possible to abstract around today (see Zulip thread here https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/struct.20conditionally.20implementing.20Drop.2FCopy)

eddyb · 2020-08-16T17:05:08Z

@Mark-Simulacrum Disallowing taking references to fields, in order to allow more intricate optimizations, has been desired for a long time, but I don't know what's been happening in this space. cc @nikomatsakis @RalfJung

As for the Copy vs Drop: we should be able to limit ourselves to Copy (or at least assert !needs_drop) for most rustc things, e.g. a lot of our arenas are Copy-only and can erase types entirely without recording destructors, as there are none to run.

src/librustc_data_structures/tagged_ptr.rs

RalfJung · 2020-08-16T17:10:29Z

Disallowing taking references to fields, in order to allow more intricate optimizations, has been desired for a long time, but I don't know what's been happening in this space.

I don't know anything about that; layout optimizations are not something I follow closely. For Miri, all I care about is that something computes layout somehow, and then we make sure everything works with that and all the right checks are done. ;)

src/librustc_data_structures/tagged_ptr.rs

Mark-Simulacrum · 2020-08-16T19:09:52Z

As for the Copy vs Drop: we should be able to limit ourselves to Copy (or at least assert !needs_drop) for most rustc things, e.g. a lot of our arenas are Copy-only and can erase types entirely without recording destructors, as there are none to run.

I am hoping to use this specifically for Rc which needs Drop impls.

nnethercote · 2020-08-16T23:28:04Z

src/librustc_middle/ty/mod.rs

+    fn into_usize(self) -> usize {
+        match self {
+            traits::Reveal::UserFacing => 0,
+            traits::Reveal::All => 1,


Can you add = 0 / = 1 at the Reveal definition, and then do self as usize instead of the match, as is done here?

I think at that point we should stick a note on Reveal and use transmute here to go back and forth, which I don't mind personally but seems like more work.

Note that you still need the "deserialization" match even with = 0/1, because you can't usize as Enum no matter what.

Let me know if you think that a transmute would be better.

I like preserving the exhaustive match here, although I assume @nnethercote's concern is the quality of generated code? I'm fine with this as-is.

nnethercote · 2020-08-16T23:28:09Z

src/librustc_middle/ty/mod.rs

+        match ptr {
+            0 => traits::Reveal::UserFacing,
+            1 => traits::Reveal::All,
+            _ => std::hint::unreachable_unchecked(),


ecstatic-morse

Left some comments from an API design perspective. In particular, I find the true const-generic parameter confusing. Since this is for internal use only, it's probably okay for it to be a messy, but I think we can do a bit better.

src/librustc_data_structures/tagged_ptr.rs

src/librustc_data_structures/tagged_ptr/copy.rs

src/librustc_data_structures/tagged_ptr.rs

ecstatic-morse · 2020-08-16T23:05:17Z

src/librustc_data_structures/tagged_ptr/copy.rs

+impl<P, T> PartialEq for CopyTaggedPtr<P, T, true>
+where
+    P: Pointer,
+    T: Tag,
+{
+    fn eq(&self, other: &Self) -> bool {
+        self.packed == other.packed
+    }
+}
+
+impl<P, T> Eq for CopyTaggedPtr<P, T, true>
+where
+    P: Pointer,
+    T: Tag,
+{
+}
+
+impl<P, T> std::hash::Hash for CopyTaggedPtr<P, T, true>
+where
+    P: Pointer,
+    T: Tag,
+{
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.packed.hash(state);
+    }
+}


I've not seen this const generics pattern before. I would have used a ShallowEq newtype wrapper instead and had deep equality by default when P: Eq to mirror the semantics of other pointer types. Is there a reason you went with this approach?

If I understand you correctly, ShallowEq would require duplicating the non-{Hash, PartialEq, Eq} impls again -- having already done that for the drop impl, I was hesitant to add another 2 sets (for Drop and for Copy). But I can if you think it's the right move.

That's true. One more suggestion: how about a ShallowEq marker trait used to specialize the PartialEq and Hash impls? This doesn't need to block this PR, however.

src/librustc_middle/ty/list.rs

src/librustc_data_structures/tagged_ptr.rs

Mark-Simulacrum · 2020-08-17T13:34:56Z

r? @ecstatic-morse -- I think I resolved your comments in the just pushed commit, or replied to them inline if I felt further discussion was warranted before changing code.

Mark-Simulacrum · 2020-08-17T13:41:36Z

@bors try @rust-timer queue

rust-timer · 2020-08-17T13:41:38Z

Awaiting bors try build completion

bors · 2020-08-17T13:41:48Z

⌛ Trying commit 5f23bf7aa38f0b4447ff39593bcc49f804092721 with merge 4b58fff90f503a8ebea6194ad38c64aa35fea076...

bors · 2020-08-17T14:28:29Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 4b58fff90f503a8ebea6194ad38c64aa35fea076 (4b58fff90f503a8ebea6194ad38c64aa35fea076)

rust-timer · 2020-08-17T14:28:31Z

Queued 4b58fff90f503a8ebea6194ad38c64aa35fea076 with parent 67e7b9b, future comparison URL.

rust-timer · 2020-08-17T18:04:49Z

Finished benchmarking try commit (4b58fff90f503a8ebea6194ad38c64aa35fea076): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

ecstatic-morse · 2020-08-19T17:59:18Z

src/librustc_data_structures/tagged_ptr.rs

+/// # Safety
+///
+/// The usize returned from `into_usize` must be a valid, dereferenceable,
+/// pointer to `<Self as Deref>::Target`. Note that pointers to `Pointee` should


Suggested change

/// pointer to `<Self as Deref>::Target`. Note that pointers to `Pointee` should

/// pointer to `<Self as Deref>::Target`. Note that pointers to `Pointee` *must*

ecstatic-morse · 2020-08-19T18:04:41Z

My broader concerns have been addressed except for the use of a const-generic boolean to select the PartialEq impl and the lack of a PartialEq impl in the case where shallow equality is incorrect. I've proposed a solution using min_specialization above that (I think) would resolve both issues. If you want to try this as part of this PR, go for it, but it could be done in a follow up as well.

If you choose to defer, r=me once the RFC 2119 nit is fixed.

Mark-Simulacrum · 2020-08-19T19:24:21Z

I was unable to come up with a scheme for ShallowEq to work how I wanted. Ideally this is our set of impls:

// currently requires both to be ShallowEq (or not), but one could imagine a full matrix of 4 options
impl<P: Pointer + PartialEq, T: Tag + PartialEq> PartialEq for TaggedPtr<P, T> {}
impl<P: Pointer + ShallowEq, T: Tag + ShallowEq> PartialEq for TaggedPtr<P, T> {}

impl<P: Pointer + Eq, T: Tag + Eq> Eq for TaggedPtr<P, T> {}
impl<P: Pointer + ShallowEq, T: Tag + ShallowEq> Eq for TaggedPtr<P, T> {}

AFAICT this set of impls isn't possible to achieve with specialization today, even in the non-min form, as you can't "de-require" a trait bound that's present on the default impl. At least I wasn't able to get it to work.

I think maybe we could have the easier to satisfy requirement that even with ShallowEq implemented you still need PartialEq/Eq, but then I wouldn't bother with specialization I think -- we can just have a const SHALLOW_EQ_AND_HASH: bool; on Pointer and Tag traits and use if statements to dispatch. We'll remove the dead code in mir opts anyway.

@bors r=ecstatic-morse

bors · 2020-08-19T19:24:22Z

📌 Commit 107e290 has been approved by ecstatic-morse

bors · 2020-08-19T20:20:36Z

⌛ Testing commit 107e290 with merge 32c654a...

bors · 2020-08-19T22:21:36Z

☀️ Test successful - checks-actions, checks-azure
Approved by: ecstatic-morse
Pushing 32c654a to master...

ecstatic-morse · 2020-08-24T21:22:29Z

Final perf results show a slight win on keccak and inflate. This is somewhat unexpected, since ParamEnv already had an ad hoc version of this optimization.

Mark-Simulacrum · 2020-08-24T21:25:59Z

It seems like pointer packing via shifting instead of or and and operations is slightly more efficient (or LLVM optimizes it better). I think that's the only difference between the two schemes (beyond splitting into a separate crate etc).

andjo403 · 2020-09-17T22:29:32Z

I think this commit seems to make the new symbol mangling to fail.
This is the first PR that fails with the error described in #76365
I do not know if the fault is here or in the symbol mangling but it as I understand it that function that is called is only used for the the size of an array and it feels strange that it shall be a bool

rust-highfive assigned ecstatic-morse Aug 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2020

RalfJung reviewed Aug 16, 2020

View reviewed changes

src/librustc_data_structures/tagged_ptr.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2020

View reviewed changes

src/librustc_data_structures/tagged_ptr.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2020

View reviewed changes

src/librustc_data_structures/tagged_ptr.rs Outdated Show resolved Hide resolved

Mark-Simulacrum force-pushed the tagged-ptr branch from b335678 to 25da9d4 Compare August 16, 2020 20:13

nnethercote reviewed Aug 16, 2020

View reviewed changes

ecstatic-morse reviewed Aug 17, 2020

View reviewed changes

ecstatic-morse reviewed Aug 19, 2020

View reviewed changes

Mark-Simulacrum added 2 commits August 19, 2020 15:08

Add tagged pointer impl to data structures

c8fe232

Use CopyTaggedPtr for ParamEnv

107e290

Mark-Simulacrum force-pushed the tagged-ptr branch from 5f23bf7 to 107e290 Compare August 19, 2020 19:08

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 19, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2020

bors merged commit 32c654a into rust-lang:master Aug 19, 2020

Mark-Simulacrum deleted the tagged-ptr branch August 24, 2020 21:23

Mark-Simulacrum mentioned this pull request Sep 17, 2020

ICE on bootstrap with new symbol mangling #76365

Closed

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a packed/tagged pointer abstraction and utilize it for ParamEnv #75590

Add a packed/tagged pointer abstraction and utilize it for ParamEnv #75590

Mark-Simulacrum commented Aug 16, 2020 •

edited

Loading

rust-highfive commented Aug 16, 2020

Mark-Simulacrum commented Aug 16, 2020

eddyb commented Aug 16, 2020

RalfJung commented Aug 16, 2020

Mark-Simulacrum commented Aug 16, 2020

nnethercote Aug 16, 2020

Mark-Simulacrum Aug 16, 2020 •

edited

Loading

ecstatic-morse Aug 19, 2020

nnethercote Aug 16, 2020

ecstatic-morse left a comment

ecstatic-morse Aug 16, 2020

Mark-Simulacrum Aug 17, 2020

ecstatic-morse Aug 19, 2020

Mark-Simulacrum commented Aug 17, 2020

Mark-Simulacrum commented Aug 17, 2020

rust-timer commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

rust-timer commented Aug 17, 2020

rust-timer commented Aug 17, 2020

ecstatic-morse Aug 19, 2020

ecstatic-morse commented Aug 19, 2020 •

edited

Loading

Mark-Simulacrum commented Aug 19, 2020

bors commented Aug 19, 2020

bors commented Aug 19, 2020

bors commented Aug 19, 2020

ecstatic-morse commented Aug 24, 2020

Mark-Simulacrum commented Aug 24, 2020

andjo403 commented Sep 17, 2020

	/// pointer to `<Self as Deref>::Target`. Note that pointers to `Pointee` should
	/// pointer to `<Self as Deref>::Target`. Note that pointers to `Pointee` must

Add a packed/tagged pointer abstraction and utilize it for ParamEnv #75590

Add a packed/tagged pointer abstraction and utilize it for ParamEnv #75590

Conversation

Mark-Simulacrum commented Aug 16, 2020 • edited Loading

rust-highfive commented Aug 16, 2020

Mark-Simulacrum commented Aug 16, 2020

eddyb commented Aug 16, 2020

RalfJung commented Aug 16, 2020

Mark-Simulacrum commented Aug 16, 2020

nnethercote Aug 16, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Aug 19, 2020

Choose a reason for hiding this comment

nnethercote Aug 16, 2020

Choose a reason for hiding this comment

ecstatic-morse left a comment

Choose a reason for hiding this comment

ecstatic-morse Aug 16, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Aug 17, 2020

Choose a reason for hiding this comment

ecstatic-morse Aug 19, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 17, 2020

Mark-Simulacrum commented Aug 17, 2020

rust-timer commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

rust-timer commented Aug 17, 2020

rust-timer commented Aug 17, 2020

ecstatic-morse Aug 19, 2020

Choose a reason for hiding this comment

ecstatic-morse commented Aug 19, 2020 • edited Loading

Mark-Simulacrum commented Aug 19, 2020

bors commented Aug 19, 2020

bors commented Aug 19, 2020

bors commented Aug 19, 2020

ecstatic-morse commented Aug 24, 2020

Mark-Simulacrum commented Aug 24, 2020

andjo403 commented Sep 17, 2020

Mark-Simulacrum commented Aug 16, 2020 •

edited

Loading

Mark-Simulacrum Aug 16, 2020 •

edited

Loading

ecstatic-morse commented Aug 19, 2020 •

edited

Loading