-
Notifications
You must be signed in to change notification settings - Fork 13k
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
librustc_mir: Propagate constants during copy propagation. #36639
Conversation
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 minor nits
MirSource::Const(_) | MirSource::Static(..) | MirSource::Promoted(..) => return, | ||
MirSource::Fn(function_node_id) => { | ||
// Don't run on const functions, because constant qualification might reject the | ||
// optimized IR. |
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.
Comment nit: not really a problem for const qualification (as that only looks at non-generic const
s only, already handled by the previous match arm), but rather rustc_trans
, which in a pre-miri world cannot evaluate MIR where mutation is used to construct ADT.
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.
@eddyb But when I tried to bootstrap without doing this then qualify_consts
started emitting errors. So I think it is a problem with that pass, no?
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.
The previous match
arm (MirSource::Const(_)
specifically) prevents qualify_consts
from freaking out. Everything else is also needed, but for a different reason (trans::mir::constant
).
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.
Added more comments.
} | ||
} | ||
|
||
impl<'tcx> MutVisitor<'tcx> for ConstantPropagationVisitor<'tcx> { |
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.
I wonder if this could be replaced with replace_all_defs_and_uses_with
taking an Operand
.
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.
At first I wanted to do that, but I realized it's not possible. That's because defs are not Operands if they appear on e.g. the LHS of an Assign.
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.
Right, but in that case, it'd be a bug!
- or you could have a separate replace_all_uses_with
.
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.
Seems like premature abstraction. What other passes will need that function? Note that it's mostly just useful for consts, because operands are either lvalues or consts.
@@ -22,12 +22,12 @@ fn main() { | |||
// START rustc.node4.PreTrans.after.mir | |||
// bb0: { | |||
// nop; // scope 0 at storage_ranges.rs:14:9: 14:10 | |||
// var0 = const 0i32; // scope 0 at storage_ranges.rs:14:13: 14:14 | |||
// nop; // scope 0 at storage_ranges.rs:14:13: 14:14 |
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 test should probably use the MIR before any optimization passes, just to check the way storage ranges are generated.
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.
Want to change this to test pre-optimization MIR? If not, r=me as-is.
bdff21a
to
cbf41b0
Compare
@bors: r=eddyb |
📌 Commit cbf41b0 has been approved by |
Wait, Travis failed, @bors r-
|
Argh, those don't run on the Mac! |
cbf41b0
to
f9b73aa
Compare
@bors: r=eddyb I think restricting the pass to MIR opt level >= 1 will fix the debug info problems. |
📌 Commit f9b73aa has been approved by |
// There must be exactly one use of the source used in a statement (not in a terminator). | ||
let src_use_info = def_use_analysis.local_info(src_local); | ||
let src_use_count = src_use_info.use_count(); | ||
if src_use_count == 0 { |
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.
Don't you have a use of src
by definition?
debug!(" Can't copy-propagate local: no uses"); | ||
return None | ||
} | ||
if src_use_count != 1 { |
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.
Why is this check needed?
error: this file contains an un-closed delimiter @bors r- |
f9b73aa
to
a1108d1
Compare
@bors: r=eddyb |
📌 Commit a1108d1 has been approved by |
⌛ Testing commit a1108d1 with merge 1cd17db... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
a1108d1
to
d1366c3
Compare
@bors: r=eddyb |
📌 Commit d1366c3 has been approved by |
⌛ Testing commit d1366c3 with merge f1278ae... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
This optimization kicks in a lot when bootstrapping the compiler.
d1366c3
to
79cb2db
Compare
@bors: r=eddyb |
📌 Commit 79cb2db has been approved by |
librustc_mir: Propagate constants during copy propagation. This optimization kicks in a lot when bootstrapping the compiler. r? @eddyb
librustc_mir: Propagate constants during copy propagation. This optimization kicks in a lot when bootstrapping the compiler. r? @eddyb
This optimization kicks in a lot when bootstrapping the compiler.
r? @eddyb