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

[MIR] Add explicit SetDiscriminant StatementKind for deaggregating enums #35348

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

scottcarr
Copy link
Contributor

@scottcarr scottcarr commented Aug 4, 2016

cc #35186

To deaggregate enums, we need to be able to explicitly set the discriminant. This PR implements a new StatementKind that does that.

I think some of the places that have panics! now could maybe do something smarter.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@scottcarr
Copy link
Contributor Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Aug 4, 2016
@nagisa
Copy link
Member

nagisa commented Aug 4, 2016

Why not a plain projection? Dealing with discriminant needn’t to be that specific.

@@ -700,7 +700,7 @@ impl<'blk, 'tcx> Drop for OwnedBuilder<'blk, 'tcx> {
}

pub struct BlockAndBuilder<'blk, 'tcx: 'blk> {
bcx: Block<'blk, 'tcx>,
pub bcx: Block<'blk, 'tcx>,
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 be done via the with_bcx method.

@eddyb
Copy link
Member

eddyb commented Aug 5, 2016

@nagisa How would that work? The discriminant is not always present in memory (e.g. nullable ptr).

@nagisa
Copy link
Member

nagisa commented Aug 5, 2016

You can still conjure a ValueRef for what would be a discriminant in a nullable pointer, although, sure, reading and writing to such projection would need a little bit special care.

@eddyb
Copy link
Member

eddyb commented Aug 5, 2016

@nagisa It's not just special care, it's not a valid lvalue. Lvalues are lvalues because they can be uniformly represented by pointers.

@nagisa
Copy link
Member

nagisa commented Aug 5, 2016

Okay, so here’re my thoughts:

First of all, it seems to me that SetDiscriminant is so special, that really no optimisation pass would be able to take advantage of it when compared to plain assign-aggregate stuff. I feel like equal amount of effort is necessary to handle this statement and the aggregate, and now you’ll need to handle them both, potentially doubling the effort, disaggregation notwithstanding.

@eddyb I still do not really see your point, so I’ll write down it for myself case-by-case (I’m using adt::trans_{set,get}_discr as a reference):

  • CEnum: Discriminant projection is the same thing as the enum lvalue. Namely, enum.discr = enum with exactly the same type too;
  • General: Discriminant projection is the same thing as a field projection. Namely, enum.discr = enum.0;
  • Univariant: Such projection is invalid (this is where your uniformity point could apply?); enum.discr = const 0; assigning non-zero constant to this aggregate is an error, reading always results in 0; If we want to support taking address of a discriminant, it might make sense to make a backing lvalue with 0 in it;
  • RawNullablePointer: Discriminant projection is the same thing as the enum lvalue. Namely enum.discr = enum with usize as a type, which needs special interpretation: 0 means variant 0, anything else is variant 1;
  • StructWrappedNullablePointer: Discriminant projection is the same thing as a field projection. Namely, struct.discr = struct.field with usize as a type, which needs special interpretation: 0 means variant 0, anything else is variant 1.

It seems to me that generating MIR for special cases I wrote down is not any more complex than generating SetDiscriminant, really, and in trans, you can still handle this projection by abusing pattern matching and matching on Assign(Projection::Discr(_), _) = statement, making the projection stuff as easy to implement as the separate SetDiscriminant statement.

@nikomatsakis
Copy link
Contributor

@nagisa initially I thought it should be a projection, but consider that you can borrow lvalues -- what you do for &x.discriminant in the case where x: Option<&T>? Or, if we ever support tagged pointers, in such a case?

This is why when @scottcarr and I were talking, we agreed that having a SetDiscriminant statement (and, perhaps later, a Discriminant rvalue) was a better approach.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 5, 2016

Ah, here is one other reason. Using a statement like this, we can very easily guarantee (via the MIR definition) that the variant is always statically known. This is important because we want things like SetDiscriminant(x, Some) to be a noop if x: Option<&T> (since, in that case, just setting the value is enough). But if it were x.discriminant = y, we'd have to dynamically test what y evaluated to.

StatementKind::SetDiscriminant{ .. } => {
// SCOTT FIXME
// is this right?
bb_ctxt.builder.create_move_path()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you call just call span_bug!(...) here. Borrowck is operating before any discriminant operations are introduced.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 9, 2016

📌 Commit 374a203 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 9, 2016

📌 Commit c39b040 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 9, 2016

⌛ Testing commit c39b040 with merge bbf055a...

@bors
Copy link
Contributor

bors commented Aug 9, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Aug 10, 2016

⌛ Testing commit c39b040 with merge 1480bcf...

@bors
Copy link
Contributor

bors commented Aug 10, 2016

💔 Test failed - auto-mac-64-opt

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2016

📌 Commit 3ada7a5 has been approved by nikomatsakis

@scottcarr scottcarr force-pushed the discriminant2 branch 2 times, most recently from 998f2f3 to e264e36 Compare August 11, 2016 17:20
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2016

📌 Commit d77a136 has been approved by nikomatsakis

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 13, 2016
…akis

[MIR] Add explicit SetDiscriminant StatementKind for deaggregating enums

cc rust-lang#35186

To deaggregate enums, we need to be able to explicitly set the discriminant.  This PR implements a new StatementKind that does that.

I think some of the places that have `panics!` now could maybe do something smarter.
@bors
Copy link
Contributor

bors commented Aug 13, 2016

⌛ Testing commit d77a136 with merge e64f688...

bors added a commit that referenced this pull request Aug 13, 2016
[MIR] Add explicit SetDiscriminant StatementKind for deaggregating enums

cc #35186

To deaggregate enums, we need to be able to explicitly set the discriminant.  This PR implements a new StatementKind that does that.

I think some of the places that have `panics!` now could maybe do something smarter.
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.

6 participants