-
Notifications
You must be signed in to change notification settings - Fork 13.3k
librustc_mir: Remove &*x
when x
has a reference type.
#36504
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
Conversation
This should open up some new copy prop opportunities. |
Does this include |
@cbreeden No, that's somewhat more complex because it's not self-contained within a statement. |
How's this? |
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 comments addressed. It might be good to also explain (in a comment?) why the optimizations are collected ahead of time instead of done in-place.
} | ||
|
||
struct OptimizationList { | ||
and_stars: HashSet<Location>, |
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.
Should use FnvHashSet
(from rustc::utils::nodemap
) in the compiler.
} | ||
|
||
impl OptimizationList { | ||
fn new() -> OptimizationList { |
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 can be done via #[derive(Default)]
.
|
||
impl<'tcx> MutVisitor<'tcx> for InstCombine { | ||
fn visit_rvalue(&mut self, rvalue: &mut Rvalue<'tcx>, location: Location) { | ||
if self.optimizations.and_stars.contains(&location) { |
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 use remove
instead of contains
to do both at once.
debug!("Replacing `&*`: {:?}", rvalue); | ||
let new_lvalue = match *rvalue { | ||
Rvalue::Ref(_, _, Lvalue::Projection(ref mut projection)) => { | ||
mem::replace(&mut projection.base, Lvalue::ReturnPointer) |
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.
Heh, nice placeholder hack.
Rvalue::Ref(_, _, Lvalue::Projection(ref mut projection)) => { | ||
mem::replace(&mut projection.base, Lvalue::ReturnPointer) | ||
} | ||
_ => panic!("Detected `&*` but didn't find `&*`!"), |
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.
Should use bug!
in the compiler.
Updated. r? @eddyb |
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.
@bors r+
@bors r+ |
📌 Commit 11a4965 has been approved by |
@bors r- This will cause drop elaboration to crash and burn, because the newly-created code moves behind |
Ah, right, my bad, I forgot that it had to go after drop elaboration. We need more comments in the pass list. |
I would like to have more understanding on what properties MIR must have at every step before we start doing destructive optimizations. |
@arielb1 Right now we are generating very bad code. I think it would be unwise to gate needed optimizations on "understanding of what properties MIR must have at every step". We need these optimizations now. As long as everything works, we can do that later. |
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 the pass reordered.
@@ -1019,6 +1019,7 @@ pub fn phase_4_translate_to_llvm<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
|
|||
passes.push_pass(box mir::transform::erase_regions::EraseRegions); | |||
|
|||
passes.push_pass(box mir::transform::instcombine::InstCombine::new()); | |||
passes.push_pass(box mir::transform::add_call_guards::AddCallGuards); | |||
passes.push_pass(box borrowck::ElaborateDrops); |
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 only need to reorder the pass to go after this line, which should be separated and commented.
This is the point where moves stop existing and you only have raw semantic-less copies.
This introduces a new `InstCombine` pass for us to place such peephole optimizations.
@bors-servo: r=eddyb |
📌 Commit e8a44d2 has been approved by |
librustc_mir: Remove `&*x` when `x` has a reference type. This introduces a new `InstCombine` pass for us to place such peephole optimizations. r? @eddyb
librustc_mir: Remove `&*x` when `x` has a reference type. This introduces a new `InstCombine` pass for us to place such peephole optimizations. r? @eddyb
This introduces a new
InstCombine
pass for us to place such peepholeoptimizations.
r? @eddyb