Skip to content

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Mar 7, 2022

#93800 caused a regression in an alt builder with parallel enabled. #94205 reverted that PR because of the regression. For an unknown reason the regression has disappeared, so we reinstate the changes in #93800 here.

r? @Mark-Simulacrum

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 7, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2022
@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented Mar 8, 2022

⌛ Trying commit bdf58b78f4a59ac0d8fee64c431d2d52f3ad929c with merge 508e803495f131ee3574ac1e64dc30b13cafa12a...

@bors
Copy link
Collaborator

bors commented Mar 8, 2022

☀️ Try build successful - checks-actions
Build commit: 508e803495f131ee3574ac1e64dc30b13cafa12a (508e803495f131ee3574ac1e64dc30b13cafa12a)

@Mark-Simulacrum
Copy link
Member

The original PR also had some mir-opt test updates, but it looks like they're not included here. Is this change different? Did something change on master?

@b-naber
Copy link
Contributor Author

b-naber commented Mar 8, 2022

The original PR also had some mir-opt test updates, but it looks like they're not included here. Is this change different? Did something change on master?

I was surprised by that as well, not sure whether anything else changed that caused the changes from the previous PR to be omitted. Maybe #94209 is the cause for this?

@lcnr and @oli-obk Do you have any idea why there aren't any diffs for the mir-opt tests?

@lcnr
Copy link
Contributor

lcnr commented Mar 8, 2022

we changed mir dumps in #94209, so it's expected that there are no changes to that anymore here

@Mark-Simulacrum
Copy link
Member

In that case I think we're likely prepared to move ahead here -- it would be nice to add a little more detail to the PR description reflecting the reasoning for the revert and the seemingly fixed regression (though through no actual changes).

r? @lcnr or @oli-obk on the actual code, though I presume that it's the same and likely still fine.

@rust-highfive rust-highfive assigned lcnr and unassigned Mark-Simulacrum Mar 8, 2022
@@ -123,7 +123,7 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, expr: &Exp
}
Closure { closure_id: _, substs: _, upvars: _, movability: _, fake_reads: _ } => {}
Literal { literal, user_ty: _, const_id: _ } => visitor.visit_const(literal),
StaticRef { literal, def_id: _ } => visitor.visit_const(literal),
StaticRef { .. } => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

please explicitly mention the ignored fields here

@lcnr
Copy link
Contributor

lcnr commented Mar 8, 2022

@bors r+ rollup=never

i guess we want to be safe here

@bors
Copy link
Collaborator

bors commented Mar 8, 2022

📌 Commit 9fd2c80 has been approved by lcnr

@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 Mar 8, 2022
@bors
Copy link
Collaborator

bors commented Mar 8, 2022

⌛ Testing commit 9fd2c80 with merge 1eb7258...

@bors
Copy link
Collaborator

bors commented Mar 8, 2022

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 1eb7258 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2022
@bors bors merged commit 1eb7258 into rust-lang:master Mar 8, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 8, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1eb7258): comparison url.

Summary: This benchmark run did not return any relevant results. 8 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants