Fix `SimplifyArmIdentity` MIR opt #94177

JakobDegen · 2022-02-20T01:57:12Z

Closes #77359
Closes #72800
Closes #78628

This is a near full re-write of the pass. The StorageLive issue would have been relatively easy to fix, but the use after move issue was more complicated. The new version of the pass documents very explicitly how copy/move things are handled, and takes care to uphold that. This version also increases the flexibility of the pass. We allow any number of fields (instead of just one) and also allow moving into ourselves.

This pass does a little less than the original version. In particular, it leaves all the storage and some of the temporary assignments in. I do think that it is in general the right idea to not try and also remove those here. I've written #94118 which is supposed to run soon after this and is designed to remove these kinds of things. Unfortunately, this also means that a lot of the tests around SimplifyBranchSame break, because that optimization no longer runs. I've gone and deleted these tests for now. I don't know if we even want to keep the SimplifyBranchSame pass at all though; this pass is enough to get LLVM to output the right thing, so it might be enough to let the remaining issues get swept up in DSE + SimplifyCFG or something like that.

The diff is pretty big, but that's mostly tests. I've organized the commits so that hopefully the changes to the test suite are pretty easy to follow.

cc @wesleywiser who seems to have been the last person to touch this

r? rust-lang/mir-opt

compiler/rustc_mir_transform/src/simplify_try.rs

JakobDegen · 2022-02-20T02:36:55Z

Sigh, this will have to wait until tomorrow

JakobDegen · 2022-02-21T15:06:04Z

Was missing a single ?. (well, was missing a #[must_use] that allowed me to miss a ?)

JakobDegen · 2022-02-21T15:29:54Z

Oop, didn't know there was a codegen test. I've modified it for now by turning on LLVM optimizations. With SimplifyBranchSame no longer running, its expected that there's still a branch in the unoptimized LLVM-IR, but LLVM does remove it.

cjgillot · 2022-02-21T16:04:31Z

compiler/rustc_mir_transform/src/simplify_try.rs

+///    bound.
+/// 2. For each bundle, if any assignment to a temporary is via `copy`, then this is sufficient to
+///    prove that the field is `Copy`. In this case, we keep all assignments to temporaries
+///    associated with this bundle. The move mode of the assignment out of `src` is forced to be


SimplifyLocals should be able to remove stray assignments. Should this pass end by a call to simplify_locals when it has left Copy assignments behind?

@cjgillot see #94118 , which would run after this, and is a smaller (and less perf intensive) hammer for the common case

Could you demonstrate what mir-opt tests look like with both optimizations running?
Other option: we are creating a list of StorageDead statements to be moved. Can it be used to kill off unused Copy locals?

Actually, I just had a different idea. If that doesn't pan out though, I'll add a test demonstrating this

JakobDegen · 2022-02-21T16:49:50Z

Hm, that test doesn't seem to run locally by default. Is there anyway to get ./x.py test src/test/mir-opt to run this? Even if it doesn't run the resulting executable, it should at least be able to generate the diff files

cjgillot · 2022-02-21T18:07:29Z

You need to specify another --target i686-unknown-linux-gnu (or another well-chosen target) to x.py for it to run 32bit mir-opts.

bors · 2022-02-22T15:25:21Z

☔ The latest upstream changes (presumably #94209) made this pull request unmergeable. Please resolve the merge conflicts.

This fixes a number of soundness issues that existed in the pass previously, and additionally makes it more flexible.

JakobDegen · 2022-02-22T19:38:16Z

@cjgillot I've added a call to simplify_locals at the end of the opt if it fired. This unfortunately degrades the quality of the tests, but I don't really see a better solution.

cjgillot · 2022-02-22T20:10:47Z

@cjgillot I've added a call to simplify_locals at the end of the opt if it fired. This unfortunately degrades the quality of the tests, but I don't really see a better solution.

Thanks.
Indeed, simplify_locals cleans up cruft left by several passes, which makes the tests diffs confusing.
What about the idea you talked about in #94177 (comment)?
What about using the StorageDead information?
Also, could you reinstate the SimplifyBranchSame tests in place?

JakobDegen · 2022-02-22T21:40:10Z

My hope is that I can also start working on dest prop and fix that so that it can run immediately after. At that point, I can remove the simplify_locals call because dest prop should pick everything up on its own. This would improve the tests and avoid issues with overhead. I can look at using information from the accumulated StorageDead as well, but I don't know if that will be worth it if dest prop is fixed.

This commit makes a couple of changes: 1. Rename `simplify-arm` to `simplify_arm` so that it appears with the output files. 2. Deduplicate tests across `simplify_arm` and `simplify_try`. This moves all the tests to `simplify_arm`, where output for both opts is emitted where appropriate. Output for `SimplifyLocals` and `DestProp` is removed, because `DestProp` is unsound and not interesting for this case anyway, while `simplify_arm` calls `simplify_locals` internally anyway 3. Blesses the `simplify_arm` tests with the new optimization. squash 1

This adds a couple more tests checking code that should be optimized, and even more checking code that shouldn't be.

JakobDegen · 2022-02-22T22:19:00Z

I've added the tests back in. Would be interested in seeing the results of a perf run of this, I don't expect it to be too expensive, but I have no idea how often it will fire.

cjgillot · 2022-02-23T18:39:44Z

@bors try @rust-timer queue

rust-timer · 2022-02-23T18:39:46Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-02-23T18:39:54Z

⌛ Trying commit dc3bc90 with merge 925b41079b7b144f270a6fd5883c7db9d5864965...

bors · 2022-02-23T20:12:35Z

☀️ Try build successful - checks-actions
Build commit: 925b41079b7b144f270a6fd5883c7db9d5864965 (925b41079b7b144f270a6fd5883c7db9d5864965)

rust-timer · 2022-02-23T20:12:37Z

Queued 925b41079b7b144f270a6fd5883c7db9d5864965 with parent c651ba8, future comparison URL.

JakobDegen · 2022-02-23T20:52:06Z

I found a comment in other code indicating that self-assignment is UB. I can push an update to this pass that avoids emitting such a self-assignment, but is that the case? If so, why?

rust-timer · 2022-02-23T22:23:00Z

Finished benchmarking commit (925b41079b7b144f270a6fd5883c7db9d5864965): comparison url.

Summary: This benchmark run shows 2 relevant improvements 🎉 but 1 relevant regression 😿 to instruction counts.

Average relevant improvement: -0.8%
Largest improvement in instruction counts: -1.4% on full builds of html5ever opt
Largest regression in instruction counts: 0.5% on incr-patched: b9b3e592dd cherry picked builds of style-servo debug

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

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

JakobDegen · 2022-03-06T03:54:38Z

I've converted this to a draft to keep it from being merged, pending #94590 . That will require some adjustments to this pass. Also there are (at least) two additional checks that need to be introduced for this to be correct: The type needs to be an ADT, not a generator, and the move into self needs to be avoided. In principle though, not a huge amount will change and this should still be ready for an initial review

JakobDegen · 2022-04-22T20:19:34Z

I have a version of this that I think is ready to review, it's only blocked on #96090 . Once that lands, I'll push updates

Mark-Simulacrum · 2022-07-24T21:41:30Z

@JakobDegen FYI: It looks like #96090 has landed at this point.

JakobDegen · 2022-07-28T00:17:48Z

Yeah, I'm still waiting for more changes though. I can close this if it's better to not have it up

JakobDegen · 2022-10-26T22:33:49Z

I'm closing this PR because it is still unsound in some very hard to fix ways.

Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts I had attempted to fix the first of these opts in rust-lang#94177 . However, despite that PR already being a full re-write, it still did not fix some of the core soundness issues. The optimizations that are attempted here are likely to be desirable, but I do not expect any of the currently written code to survive into a sound implementation. Deleting the code keeps us from having to maintain the passes in the meantime. Closes rust-lang#77359 , closes rust-lang#72800 , closes rust-lang#78628 r? `@cjgillot`

Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts I had attempted to fix the first of these opts in rust-lang#94177 . However, despite that PR already being a full re-write, it still did not fix some of the core soundness issues. The optimizations that are attempted here are likely to be desirable, but I do not expect any of the currently written code to survive into a sound implementation. Deleting the code keeps us from having to maintain the passes in the meantime. Closes rust-lang#77359 , closes rust-lang#72800 , closes rust-lang#78628 r? ``@cjgillot``

Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts I had attempted to fix the first of these opts in rust-lang#94177 . However, despite that PR already being a full re-write, it still did not fix some of the core soundness issues. The optimizations that are attempted here are likely to be desirable, but I do not expect any of the currently written code to survive into a sound implementation. Deleting the code keeps us from having to maintain the passes in the meantime. Closes rust-lang#77359 , closes rust-lang#72800 , closes rust-lang#78628 r? ```@cjgillot```

rust-highfive assigned wesleywiser Feb 20, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2022

JakobDegen commented Feb 20, 2022

View reviewed changes

compiler/rustc_mir_transform/src/simplify_try.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

JakobDegen force-pushed the fix-simplify-try branch from d406e0e to 85ebacc Compare February 21, 2022 15:05

This comment has been minimized.

Sign in to view

JakobDegen force-pushed the fix-simplify-try branch from 85ebacc to 6070f63 Compare February 21, 2022 15:26

cjgillot reviewed Feb 21, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Rewrite SimplifyArmIdentity pass.

3b06220

This fixes a number of soundness issues that existed in the pass previously, and additionally makes it more flexible.

JakobDegen force-pushed the fix-simplify-try branch from 6070f63 to e59021d Compare February 22, 2022 19:02

JakobDegen added 3 commits February 22, 2022 17:13

Extend the tests for SimplifyArmIdentity.

8af6511

This adds a couple more tests checking code that should be optimized, and even more checking code that shouldn't be.

Fix other tests broken by change to SimplifyArmIdentity

dc3bc90

JakobDegen force-pushed the fix-simplify-try branch from e59021d to dc3bc90 Compare February 22, 2022 22:17

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 23, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 23, 2022

JakobDegen mentioned this pull request Mar 4, 2022

Clarify semantics of SetDiscriminant and change enum deaggregation to match those semantics #94590

Closed

JakobDegen marked this pull request as draft March 6, 2022 03:53

Alexendoo mentioned this pull request Apr 12, 2022

ICE: broken mir with -Zunsound-mir-opts: simplify arm identity #78628

Closed

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022

JakobDegen closed this Oct 26, 2022

JakobDegen deleted the fix-simplify-try branch January 1, 2023 21:28

JakobDegen mentioned this pull request Jan 24, 2023

Delete SimplifyArmIdentity and SimplifyBranchSame mir opts #107256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `SimplifyArmIdentity` MIR opt #94177

Fix `SimplifyArmIdentity` MIR opt #94177

JakobDegen commented Feb 20, 2022

This comment has been minimized.

JakobDegen commented Feb 20, 2022

JakobDegen commented Feb 21, 2022

This comment has been minimized.

JakobDegen commented Feb 21, 2022

cjgillot Feb 21, 2022

JakobDegen Feb 21, 2022

cjgillot Feb 21, 2022

JakobDegen Feb 21, 2022

This comment has been minimized.

JakobDegen commented Feb 21, 2022

cjgillot commented Feb 21, 2022

bors commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

cjgillot commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

cjgillot commented Feb 23, 2022

rust-timer commented Feb 23, 2022

bors commented Feb 23, 2022

bors commented Feb 23, 2022

rust-timer commented Feb 23, 2022

JakobDegen commented Feb 23, 2022

rust-timer commented Feb 23, 2022

JakobDegen commented Mar 6, 2022 •

edited

Loading

JakobDegen commented Apr 22, 2022

Mark-Simulacrum commented Jul 24, 2022

JakobDegen commented Jul 28, 2022

JakobDegen commented Oct 26, 2022

Fix SimplifyArmIdentity MIR opt #94177

Fix SimplifyArmIdentity MIR opt #94177

Conversation

JakobDegen commented Feb 20, 2022

This comment has been minimized.

JakobDegen commented Feb 20, 2022

JakobDegen commented Feb 21, 2022

This comment has been minimized.

JakobDegen commented Feb 21, 2022

cjgillot Feb 21, 2022

Choose a reason for hiding this comment

JakobDegen Feb 21, 2022

Choose a reason for hiding this comment

cjgillot Feb 21, 2022

Choose a reason for hiding this comment

JakobDegen Feb 21, 2022

Choose a reason for hiding this comment

This comment has been minimized.

JakobDegen commented Feb 21, 2022

cjgillot commented Feb 21, 2022

bors commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

cjgillot commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

JakobDegen commented Feb 22, 2022

cjgillot commented Feb 23, 2022

rust-timer commented Feb 23, 2022

bors commented Feb 23, 2022

bors commented Feb 23, 2022

rust-timer commented Feb 23, 2022

JakobDegen commented Feb 23, 2022

rust-timer commented Feb 23, 2022

JakobDegen commented Mar 6, 2022 • edited Loading

JakobDegen commented Apr 22, 2022

Mark-Simulacrum commented Jul 24, 2022

JakobDegen commented Jul 28, 2022

JakobDegen commented Oct 26, 2022

Fix `SimplifyArmIdentity` MIR opt #94177

Fix `SimplifyArmIdentity` MIR opt #94177

JakobDegen commented Mar 6, 2022 •

edited

Loading