-
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
add visit_operand
to const prop
#74507
Conversation
+ // + val: Value(Scalar(0x00000002)) | ||
+ // mir::Constant | ||
+ // + span: $DIR/array_index.rs:5:18: 5:33 | ||
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) } |
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.
oh god these comments are really annoying 😆
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 really wish there rwas a way to suppress all the comments in MIR dumps
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.
@@ -56,7 +56,14 @@ | |||
} | |||
|
|||
bb2: { | |||
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30 | |||
- switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30 | |||
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30 |
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.
awesome! later passes will replace this with a goto
now
@bors try @rust-timer build let's do a perf run so we know whether we can let it get rolled up or not |
⌛ Trying commit 9ea645fc8ea275b1a5b868258fe6021c6ac36578 with merge 23214cf1fdf15e1a92e0d297e6de7123d713c9ff... |
☀️ Try build successful - checks-actions, checks-azure |
Queued 23214cf1fdf15e1a92e0d297e6de7123d713c9ff with parent 47ea6d9, future comparison URL. |
Finished benchmarking try commit (23214cf1fdf15e1a92e0d297e6de7123d713c9ff): comparison url. |
} | ||
TerminatorKind::SwitchInt { ref mut discr, .. } => { | ||
// FIXME: This is currently redundant with `visit_operand`, but sadly | ||
// always visiting operands currently causes a perf regression, so |
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.
could be worth mentioning the perf regression is not due to the cost of actually visiting the operands but the changes it causes to MIR code
r=me with more info on the perf regression on that comment |
referenced LLVM in the fixme, not sure if you want anything else mentioned. |
@bors r+ |
📌 Commit d257bac has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
add `visit_operand` to const prop r? @oli-obk
add `visit_operand` to const prop r? @oli-obk
add `visit_operand` to const prop r? @oli-obk
Should this be rollup=never? |
Not sure, this shouldn't change anything for |
⌛ Testing commit d257bac with merge 88172d158a88b12df391690503ba19bcb7b18e19... |
@bors r- |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
LGTM; let's see what @oli-obk says. |
this visits unevaluated constants and potentially evaluates them again, so that change is definitely an improvement @bors r+ |
📌 Commit 61a9ab8 has been approved by |
add `visit_operand` to const prop r? @oli-obk
@bors treeclosed- |
☀️ Test successful - checks-actions, checks-azure |
r? @oli-obk