-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Remove adt_def from projections and downcasts in MIR #59514
Remove adt_def from projections and downcasts in MIR #59514
Conversation
@bors try |
…=<try> Remove adt_def from projections and downcasts in MIR As part of optimizing generator layouts in MIR, we'd like to allow downcasting generators to variants which do not have a corresponding `def_id`, since they are created by the compiler. This refactor hopes to allow that, without regressing perf. r? @eddyb
f4a1628
to
d1b02f7
Compare
The job Click to expand the log.
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 |
d1b02f7
to
2ff7e78
Compare
@rust-timer build 24a40d8 |
Success: Queued 24a40d8 with parent 70a497a, comparison URL. |
Finished benchmarking try commit 24a40d8 |
src/librustc/mir/tcx.rs
Outdated
ProjectionElem::Downcast(_name, index) => { | ||
let ty = self.to_ty(); | ||
match &ty.sty { | ||
ty::Adt(adt_def, _substs) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this match
, it's kind of pointless nowadays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
let variant_index = match &self { | ||
PlaceTy::Ty { .. } => VariantIdx::new(0), | ||
PlaceTy::Downcast { ty: _, variant_index } => *variant_index, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is subtly incomplete, but pre-existing.
The non-downcast case should use .non_enum_variant()
to get the variant, while downcasts should assert the ADT is an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1980,16 +1980,16 @@ pub enum ProjectionElem<'tcx, V, T> { | |||
/// "Downcast" to a variant of an ADT. Currently, we only introduce | |||
/// this for ADTs with more than one variant. It may be better to | |||
/// just introduce it always, or always for enums. | |||
Downcast(&'tcx AdtDef, VariantIdx), | |||
Downcast(Option<Symbol>, VariantIdx), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, how do we print fields? Always by index? Or do they use some trickery to show the name of the field? Because if the latter, we can reuse that for downcasts too.
src/librustc/mir/tcx.rs
Outdated
@@ -16,8 +16,7 @@ pub enum PlaceTy<'tcx> { | |||
Ty { ty: Ty<'tcx> }, | |||
|
|||
/// Downcast to a particular variant of an enum. | |||
Downcast { adt_def: &'tcx AdtDef, | |||
substs: SubstsRef<'tcx>, | |||
Downcast { ty: Ty<'tcx>, | |||
variant_index: VariantIdx }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are going ahead with this, might as well switch to:
pub struct PlaceTy<'tcx> {
pub ty: Ty<'tcx>,
pub variant_index: Option<VariantIdx>,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
2ff7e78
to
943a21b
Compare
src/librustc/mir/tcx.rs
Outdated
tcx.mk_adt(adt_def, substs), | ||
} | ||
pub fn to_ty(&self) -> Ty<'tcx> { | ||
self.ty | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this?
src/librustc/mir/tcx.rs
Outdated
let answer = match self.ty.sty { | ||
ty::TyKind::Adt(adt_def, substs) => { | ||
let variant_def = match &self.variant_index { | ||
None => &adt_def.non_enum_variant(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary borrows in these two lines.
let variant = if let Some(idx) = variant_index { | ||
&def.variants[idx] | ||
} else { | ||
if def.is_enum() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be impossible.
3249324
to
ae0e763
Compare
Updated, then rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with this last comment addressed
ae0e763
to
4122d22
Compare
@bors r=eddyb |
📌 Commit 4122d22 has been approved by |
…ion-elem, r=eddyb Remove adt_def from projections and downcasts in MIR As part of optimizing generator layouts in MIR, we'd like to allow downcasting generators to variants which do not have a corresponding `def_id`, since they are created by the compiler. This refactor hopes to allow that, without regressing perf. r? @eddyb
…ion-elem, r=eddyb Remove adt_def from projections and downcasts in MIR As part of optimizing generator layouts in MIR, we'd like to allow downcasting generators to variants which do not have a corresponding `def_id`, since they are created by the compiler. This refactor hopes to allow that, without regressing perf. r? @eddyb
As part of optimizing generator layouts in MIR, we'd like to allow downcasting generators to variants which do not have a corresponding
def_id
, since they are created by the compiler.This refactor hopes to allow that, without regressing perf.
r? @eddyb