Skip to content

add visit_operand to const prop#74507

Merged
bors merged 3 commits intorust-lang:masterfrom
lcnr:const-prop-into-op
Jul 24, 2020
Merged

add visit_operand to const prop#74507
bors merged 3 commits intorust-lang:masterfrom
lcnr:const-prop-into-op

Conversation

@lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 19, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2020
oli-obk
oli-obk reviewed Jul 19, 2020
View reviewed changes
src/test/mir-opt/const_prop/array_index/32bit/rustc.main.ConstProp.diff Outdated
Copy link
Contributor

@oli-obk oli-obk Jul 19, 2020

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 😆

Copy link
Contributor

@xldenis xldenis Jul 22, 2020

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

Copy link
Contributor

@oli-obk oli-obk Jul 22, 2020

Choose a reason for hiding this comment

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

#74508

oli-obk
oli-obk reviewed Jul 19, 2020
View reviewed changes
src/test/mir-opt/const_prop/discriminant/32bit/rustc.main.ConstProp.diff Outdated
Copy link
Contributor

@oli-obk oli-obk Jul 19, 2020

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 19, 2020

@bors try @rust-timer build

let's do a perf run so we know whether we can let it get rolled up or not

@bors
Copy link
Collaborator

bors commented Jul 19, 2020

⌛ Trying commit 9ea645fc8ea275b1a5b868258fe6021c6ac36578 with merge 23214cf1fdf15e1a92e0d297e6de7123d713c9ff...

@bors
Copy link
Collaborator

bors commented Jul 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 23214cf1fdf15e1a92e0d297e6de7123d713c9ff (23214cf1fdf15e1a92e0d297e6de7123d713c9ff)

@rust-timer
Copy link
Collaborator

rust-timer commented Jul 19, 2020

Queued 23214cf1fdf15e1a92e0d297e6de7123d713c9ff with parent 47ea6d9, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Jul 19, 2020

Finished benchmarking try commit (23214cf1fdf15e1a92e0d297e6de7123d713c9ff): comparison url.

@lcnr lcnr force-pushed the const-prop-into-op branch from 2205e4c to 1f0b9c3 Compare July 21, 2020 21:48
oli-obk
oli-obk reviewed Jul 22, 2020
View reviewed changes
@lcnr lcnr force-pushed the const-prop-into-op branch from 1f0b9c3 to c74c648 Compare July 22, 2020 09:32
xldenis
xldenis reviewed Jul 22, 2020
View reviewed changes
src/librustc_mir/transform/const_prop.rs Outdated
}
TerminatorKind::SwitchInt { ref mut discr, .. } => {
// FIXME: This is currently redundant with `visit_operand`, but sadly
// always visiting operands currently causes a perf regression, so
Copy link
Contributor

@xldenis xldenis Jul 22, 2020

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 22, 2020

r=me with more info on the perf regression on that comment

@lcnr
Copy link
Contributor Author

lcnr commented Jul 22, 2020

referenced LLVM in the fixme, not sure if you want anything else mentioned.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 22, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 22, 2020

📌 Commit d257bac has been approved by oli-obk

@bors
Copy link
Collaborator

bors commented Jul 22, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
@Manishearth
Rollup merge of rust-lang#74507 - lcnr:const-prop-into-op, r=oli-obk
6690992 
add `visit_operand` to const prop

r? @oli-obk
@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
@Manishearth
Rollup merge of rust-lang#74507 - lcnr:const-prop-into-op, r=oli-obk
894a76c 
add `visit_operand` to const prop

r? @oli-obk
@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
@Manishearth
Rollup merge of rust-lang#74507 - lcnr:const-prop-into-op, r=oli-obk
792d485 
add `visit_operand` to const prop

r? @oli-obk
@Manishearth
Copy link
Member

Manishearth commented Jul 22, 2020

Should this be rollup=never?

@lcnr
Copy link
Contributor Author

lcnr commented Jul 22, 2020 •
edited
Loading

Not sure, this shouldn't change anything for mir-opt-level < 2 which is the default so I would be surprised if this has a perf impact

RalfJung
RalfJung reviewed Jul 23, 2020
View reviewed changes
@bors
Copy link
Collaborator

bors commented Jul 23, 2020

⌛ Testing commit d257bac with merge 88172d158a88b12df391690503ba19bcb7b18e19...

@lcnr
Copy link
Contributor Author

lcnr commented Jul 23, 2020

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 23, 2020
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 23, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2020
@RalfJung
Copy link
Member

RalfJung commented Jul 24, 2020

LGTM; let's see what @oli-obk says.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2020

this visits unevaluated constants and potentially evaluates them again, so that change is definitely an improvement

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 24, 2020

📌 Commit 61a9ab8 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jul 24, 2020
@Dylan-DPC
Rollup merge of rust-lang#74507 - lcnr:const-prop-into-op, r=oli-obk
1279137 
add `visit_operand` to const prop

r? @oli-obk
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Jul 24, 2020
Closed
@Manishearth
Copy link
Member

Manishearth commented Jul 24, 2020

@bors treeclosed-

@bors
Copy link
Collaborator

bors commented Jul 24, 2020

⌛ Testing commit 61a9ab8 with merge d8cf749...

@bors
Copy link
Collaborator

bors commented Jul 24, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing d8cf749 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 24, 2020
@bors bors merged commit d8cf749 into rust-lang:master Jul 24, 2020
@lcnr lcnr deleted the const-prop-into-op branch July 24, 2020 23:13
@bugadani bugadani mentioned this pull request Sep 23, 2020
Merged
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

@oli-obk oli-obk oli-obk left review comments

+1 more reviewer

@xldenis xldenis xldenis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@oli-obk oli-obk

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.47.0

Development

Successfully merging this pull request may close these issues.

10 participants

@lcnr @oli-obk @bors @rust-timer @Manishearth @rust-log-analyzer @RalfJung @xldenis @cuviper @rust-highfive