Add option to mir::MutVisitor to not invalidate CFG.

[JakobDegen]

This also applies that option to some uses of the visitor. I had considered a design more similar to #100087 in which we detect if the CFG needs to be invalidated, but that is more difficult with the visitor API and so I decided against it. Another alternative to this design is to offer an API for "saving" and "restoring" CFG caches across arbitrary code. Such an API is more general, and so we may eventually want it anyway, but it seems overkill for this use case.

r? @tmiasko

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[JakobDegen]

Thanks for the suggestions!

@rustbot ready

[tmiasko]

At some point it might be worth adding a validation of cached information behind -Zvalidate-mir.

Looks good, thanks!

@bors delegate+

[bors]

✌️ @JakobDegen can now approve this pull request

[JakobDegen]

At some point it might be worth adding a validation of cached information behind -Zvalidate-mir.

Yes, definitely. At some point I want to add stateful analyses in the pass manager, at which point we'll definitely need this, so I'm kind of putting it off until then.

[JakobDegen]

@bors r=tmiasko rollup=never

Potentially perf sensitive

[bors]

📌 Commit 7547084 has been approved by tmiasko

It is now in the queue for this repository.

[bors]

⌛ Testing commit 7547084 with merge cc4dd6f...

[bors]

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing cc4dd6f to master...

[rust-timer]

Finished benchmarking commit (cc4dd6f): comparison url.

Instruction count

• Primary benchmarks: ❌ relevant regressions found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions ❌ (primary)	0.3%	0.4%	3
Regressions ❌ (secondary)	1.3%	1.5%	6
Improvements ✅ (primary)	N/A	N/A	0
Improvements ✅ (secondary)	-1.5%	-1.5%	1
All ❌✅ (primary)	0.3%	0.4%	3

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: ❌ relevant regression found

	mean1	max	count2
Regressions ❌ (primary)	N/A	N/A	0
Regressions ❌ (secondary)	3.1%	3.1%	1
Improvements ✅ (primary)	N/A	N/A	0
Improvements ✅ (secondary)	N/A	N/A	0
All ❌✅ (primary)	N/A	N/A	0

Cycles

Results

• Primary benchmarks: ✅ relevant improvement found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions ❌ (primary)	N/A	N/A	0
Regressions ❌ (secondary)	N/A	N/A	0
Improvements ✅ (primary)	-4.5%	-4.5%	1
Improvements ✅ (secondary)	N/A	N/A	0
All ❌✅ (primary)	-4.5%	-4.5%	1

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[rylev]

This is a small enough regression that I don't think we need to investigate it deeply. I ran cachgrind diff, and I didn't see anything that really stood out to me although I can't say I understand this part of the code base deeply.

 7,877,684  ???:<rustc_middle::ty::fast_reject::DeepRejectCtxt>::types_may_unify
 3,047,609  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_ty
-2,262,117  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_middle::ty::subst::SubstFolder>
 1,473,070  ???:<rustc_middle::ty::ParamEnvAnd<rustc_middle::traits::query::type_op::ProvePredicate> as rustc_trait_selection::traits::query::type_op::TypeOp>::fully_perform
  -921,526  ???:<rustc_infer::infer::InferCtxt>::instantiate_nll_query_response_and_region_obligations::<()>
  -649,700  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_middle::ty::fold::TypeFoldable>::try_fold_with::<rustc_middle::ty::subst::SubstFolder>
   462,905  ???:<alloc::rc::Rc<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree>> as core::ops::drop::Drop>::drop

It seems there's more calls in type unification, but I don't see how this change would impact that. Let me know if you have any ideas. Until then, I'm marking this as triaged.

@rustbot label +perf-regression-triaged

[JakobDegen]

This is definitely noise.

This PR is doing strictly less work than before, I was possibly expecting an improvement, but don't see how this could cause a slowdown.

[nnethercote]

I agree that this looks like noise. The code in and around fold_ty is hot and sensitive to minor changes in inlining. If you check "show non-relevant results" you see there are all sorts of ups and downs, and the net average is a 0.01% improvement. For keccak we see several 2%+ improvements, but for some reason the significance threshold for keccak has gotten really high and so these don't show up by default.

[JakobDegen]

Oh interesting. That makes sense actually, keccak is pretty heavy on the MIR infrastructure so that's one of the ones I would've expected to disproportionately benefit from this

[lqd]

keccak has been kinda noisy recently, right ?

The significance thresholding didn't find this 5% regression relevant either.