Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…r=<try> MIR move elimination [1.5/6]: Ensure ZSTs are always initialized
This comment was marked as outdated.
This comment was marked as outdated.
104da00 to
b25b42e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ddd9a7e): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.6%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.9%, secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.629s -> 489.137s (0.10%) |
This comment has been minimized.
This comment has been minimized.
|
I investigated the perf results: they are entirely due to different CGU partitioning. Retaining ZST assignments affects function size estimates. The actual ZST assignments never make it to LLVM codegen. |
This comment has been minimized.
This comment has been minimized.
34e20b4 to
2f4b8da
Compare
| @@ -1,4 +1,4 @@ | |||
| //@ compile-flags: -O -Zmerge-functions=disabled | |||
| //@ compile-flags: -O -Zmerge-functions=disabled -Zinline-mir-threshold=52 | |||
There was a problem hiding this comment.
I'm not sure how to properly address this one: it seems to be very sensitive to the MIR inliner threshold. Adding a _0 = () causes it to no longer inline, which fails the test. I can bypass that by raising the threshold from 50 to 52, but this feels like the wrong solution here.
There was a problem hiding this comment.
We can treat _0 = () as a zero-cost statement in the inliner, since it's a no-op.
There was a problem hiding this comment.
I'm happy to do that, but I'm just not sure whether special-casing this is the correct approach. We could alternatively just raise the default threshold by 5.
This comment has been minimized.
This comment has been minimized.
2f4b8da to
c1901f7
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Split off from #163335
The new MIR semantics from rust-lang/rfcs#3943 require that a local be initialized before it is used, since it only gains an allocation at that point. This means that ZSTs must be initialized before being read, even though the initialization is a no-op in codegen.
This PR addresses this in 2 ways:
RemoveZstspass to only remove ZST assignments when the destination is indirect, since such places are required to already be allocated anyways. This is fine in practice since dead ZST assignments are later removed by DSE.The last one also exposed a limitation in
SimplifyMatch's handling of constant equality when checking if two match arms are equivalent: it was only checking for scalar constants and was not handling()unit constants, which resulted in a test regression. The pass has been fixed to handle any kind ofConstValue.r? tmiasko