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 unified ADTs #30890

Merged
merged 4 commits into from
Jan 15, 2016
Merged

MIR unified ADTs #30890

merged 4 commits into from
Jan 15, 2016

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jan 14, 2016

This PR changes translation of tuple-like ADTs from being calls to being proper aggregates. This change is done in hope to make code generation better. Namely, now we can avoid:

  1. Call overhead;
  2. Generating landingpads in presence of cleanups (we know for sure constructing ADTs can’t panic);
  3. And probably much more, gaining better MIR introspectablilty.

Along with that a few serious deficiencies with translation of ADTs and switches have been fixed as well (commits 2 and 3).

r? @nikomatsakis
cc @tsion

}
} else {
ExprKind::Call {
ty: &cx.tcx.node_id_to_type(fun.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 was present before, but I think this & is redundant and will just get deref-coerced out of existence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, probably a remnant of when the code was generic over the HIR.

@nikomatsakis
Copy link
Contributor

Can you add a few more tests:

  • using a tuple variable as a value (a fn value), e.g., let x = Some; let y = x(3); y.unwrap()
  • struct tuple variants

Otherwise, looks good to me.

@nikomatsakis
Copy link
Contributor

To be clear, r=me with more tests.

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 14, 2016

📌 Commit b9d54b6 has been approved by nikomatsakis

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors r=nikomatsakis 949dc72

(had omitted a rustc_mir attribute on test)

@arielb1
Copy link
Contributor

arielb1 commented Jan 14, 2016

We might want a more generic infrastructure to handle things like intrinsics, and maybe even trivial Deref functions.

@bors
Copy link
Contributor

bors commented Jan 15, 2016

⌛ Testing commit 949dc72 with merge fef6ad1...

@bors
Copy link
Contributor

bors commented Jan 15, 2016

💔 Test failed - auto-mac-64-nopt-t

Previously we would generate regular calls for these, which is likely to result in worse LLVM code,
especially in presence of cleanups – we needn’t unecessarilly generate landing pads to construct an
ADT!
@nagisa nagisa force-pushed the mir-tuple-adts branch 2 times, most recently from bb50c48 to 72eacc1 Compare January 15, 2016 16:11
@nagisa
Copy link
Member Author

nagisa commented Jan 15, 2016

@bors r=nikomatsakis 72eacc1

(merge conflict with another patch which was not realised by git)

Previously it would go through def_id and retrieve a type that’s not always correct, monomorphized,
etc.
These tests test building and matching some non-trivial ADT configurations such as C-like enums and
packed structs.
@nagisa
Copy link
Member Author

nagisa commented Jan 15, 2016

@bors r=nikomatsakis 7d6da8e

(same reason, didn’t pay enough attention when dealing with the conflict)

bors added a commit that referenced this pull request Jan 15, 2016
This PR changes translation of tuple-like ADTs from being calls to being proper aggregates. This change is done in hope to make code generation better. Namely, now we can avoid:

1. Call overhead;
2. Generating landingpads in presence of cleanups (we know for sure constructing ADTs can’t panic);
3. And probably much more, gaining better MIR introspectablilty.

Along with that a few serious deficiencies with translation of ADTs and switches have been fixed as well (commits 2 and 3).

r? @nikomatsakis
cc @tsion
@bors
Copy link
Contributor

bors commented Jan 15, 2016

⌛ Testing commit 7d6da8e with merge f4befa6...

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.

5 participants