Let MonoReachable traversal evaluate BinOps

[saethlin]

This fixes the first bug in #129282

I don't know what the right organization is for the code here, but something probably has to get rearranged because this traversal now depends on rustc_const_eval so that it can use an InterpCx internally, so it can't live in rustc_middle.

r? @ghost

[saethlin]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 93ae299 with merge 448299e...

[bors]

💔 Test failed - checks-actions

[scottmcm]

Pondering: Can we just tell people that they have to write this in a const block if they want this? It's not obvious to me that doing a bunch of extra const eval in mono is worth it.

(AKA "we should have const if and a lint encouraging its use in certain places".)

[saethlin]

Pondering: Can we just tell people that they have to write this in a const block if they want this?
(AKA "we should have const if

I'm not interested in changing or adding to the language. If you want to do that, go right ahead, but it's not my thing. This seems like a useful optimization for the compiler to do, so I'm trying to see how that turns out.

I think the implementation ends up convoluted because of some deeper issues we have that I'm not going to solve. MIR is really not easy to ad-hoc evaluate, and also we don't have any post-mono MIR opts. Post-mono GVN for example would fix this. I'm pasting most of the impl out of GVN to do this.

@bors try @rust-timer queue

[bors]

⌛ Trying commit 10b7e0e with merge 012c415...

[bors]

☀️ Try build successful - checks-actions
Build commit: 012c415 (012c415c2205b3cc9c6f0a157b6c7b45f4418fc4)

[rust-timer]

Finished benchmarking commit (012c415): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 749.351s -> 750.569s (0.16%)
Artifact size: 338.82 MiB -> 338.89 MiB (0.02%)

[RalfJung]

I'm not interested in changing or adding to the language.

It shouldn't be needed to change the language. This is stable:

  if const { SIZE < 4096 } {
    let arr = [0u8; SIZE];
    std::hint::black_box(&arr);
  } else {
    let vec = vec![0u8; SIZE];
    std::hint::black_box(&vec);
  }

If that works, IMO there's no sufficient justification for piling on hacks like this.

[RalfJung]

Seems like that doesn't work. We should figure out why. I don't think we want to start integrating general optimizations into the monomorphization step -- if we decide we need post-mono optimizations on the MIR level, we should do it properly and generate MIR for that, not embed a bunch of hard-to-audit on-the-fly transformations hidden in various other passes.

[saethlin]

We should figure out why.

We already know why: #129283

if we decide we need post-mono optimizations on the MIR level, we should do it properly and generate MIR for that,

This would require a broader discussion about either deciding how much compile time regression is acceptable or ripping out out codegen canonicalization from MIR to reduce it. I demonstrated that one of those is required in #125025

[RalfJung]

We already know why: #129283

So #129283 plus using const { ... } is enough to fix #129282? IMO that's enough. It's a debug build, after all, it's not even asking to be optimized.

[saethlin]

there's no sufficient justification for piling on hacks like this.

This PR is an experiment. Unfortunately I can't collect perf results without posting a PR. I've set the status to "experimental" instead of "waiting on author" is that more clear?

[RalfJung]

Fair. :)

[cjgillot]

Should we just make this a variant for mir::Const and avoid having this logic?

[RalfJung]

No, that would be really odd.

We could consider making UbChecks a const (giving it a DefId), similar to how we are making nullary intrinsics effectively consts. However, UbChecks is not actually a const: its value depends on tcx.sess which can have different values in different crates across the crate graph!

[saethlin]

The merge conflicts here are a mess, so I don't plan on fixing them. I think the way to get such an operation merged is by doing post-mono GVN or something like that.