[mir-opt] asking ?s in a more optimized fashion
#66282
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 91e5a0e with merge fb1a35ea56157ef7f76c1451f72cda0b3be86a38... |
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors retry try |
[WIP] [mir-opt] asking `?`s in a more optimized fashion This PR works towards #66234 by providing two optimization passes meant to run in sequence: - `SimplifyArmIdentity` which transforms something like: ```rust _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP; discriminant(_LOCAL_0) = VAR_IDX; ``` into: ```rust _LOCAL_0 = move _LOCAL_1 ``` - `SimplifyBranchSame` which transforms `SwitchInt`s to identical basic blocks into a `goto` to the first reachable target. Together, these are meant to simplify the following into just `res`: ```rust match res { Ok(x) => Ok(x), Err(x) => Err(x), } ``` It should be noted however that the desugaring of `?` includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on `x`. Inlining requires `mir-opt-level=2` so this might not have any effect in perf-bot but let's find out. r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)
| // scope 6 { | ||
| // } | ||
| // bb0: { | ||
| // _2 = discriminant(_1); |
There was a problem hiding this comment.
This is a tad unfortunate that _2 isn't optimized out.
There was a problem hiding this comment.
I thought we had some sort of DCE. If not, open an issue so we create it
There was a problem hiding this comment.
I think this will be easy to fix. I'll open a PR tonight with that.
There was a problem hiding this comment.
There are some ppl at rustfest working on it
There was a problem hiding this comment.
Does that refer to #66329 ? -- That won't help in this case since this is an unused, but reachable, local.
There was a problem hiding this comment.
No, different optimization, not sure on what repo it's being developed
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-azure |
|
Queued 0113342 with parent e2fa952, future comparison URL. |
|
Finished benchmarking try commit 0113342, comparison URL. |
This comment has been minimized.
This comment has been minimized.
Centril
force-pushed
the
simplify-try
branch
from
219d24f to
f026031
Compare
Centril
changed the title
?s in a more optimized fashion?s in a more optimized fashion
|
Adjusted the existing Me and Oliver have been discussing some generalizations to these optimizations (for follow ups). In particular, we can generalize the second one into two passes:
Finally, the |
|
Actually, looking at |
Centril
added
S-waiting-on-review
|
This should be ready now :) |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 2f00e86 with merge ae731e69b0e519dbf73d4a870a82224b778808de... |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
@bors retry spurious |
bors
added
S-waiting-on-bors
[mir-opt] asking `?`s in a more optimized fashion This PR works towards #66234 by providing two optimization passes meant to run in sequence: - `SimplifyArmIdentity` which transforms something like: ```rust _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP; discriminant(_LOCAL_0) = VAR_IDX; ``` into: ```rust _LOCAL_0 = move _LOCAL_1 ``` - `SimplifyBranchSame` which transforms `SwitchInt`s to identical basic blocks into a `goto` to the first reachable target. Together, these are meant to simplify the following into just `res`: ```rust match res { Ok(x) => Ok(x), Err(x) => Err(x), } ``` It should be noted however that the desugaring of `?` includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on `x`. Inlining requires `mir-opt-level=2` so this might not have any effect in perf-bot but let's find out. r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)
|
☀️ Test successful - checks-azure |
|
@oli-obk sorry wrong pr 😂 |
| || Some((local_0, vf_s0.var_idx)) != match_set_discr(s2) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
You're not checking that local_0 != local_1, which is another soundness condition.
cc @rust-lang/wg-mir-opt
| /// | ||
| /// ```rust | ||
| /// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY ); | ||
| /// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP; |
There was a problem hiding this comment.
This is only sound to collapse if _LOCAL_TMP isn't used anywhere else (see promote_consts for an example of an analysis like that).
But also, there's another issue here... namely, debuginfo.
You need to replace _LOCAL_TMP with ((_LOCAL_1 as Variant ).FIELD: TY) in debuginfo as well, making this a very specialized form of copy-prop, I guess?
cc @rust-lang/wg-mir-opt
There was a problem hiding this comment.
It would be nice to preserve debuginfo perfectly, but for that we might need a way to merge scopes or something, so that SourceInfo from all 3 statements is combined.
Effectively, the resulting _LOCAL_0 = move _LOCAL_1 needs to be in a superposition of all the scopes.
Which might be hard due to the inlining.
| _ => unreachable!(), | ||
| } | ||
| s1.make_nop(); | ||
| s2.make_nop(); |
There was a problem hiding this comment.
I would replace the third statement and nop the first two. That way, you get the SourceInfo of the Ok(x) rather than the x use (or whatever initialized it).
| /// Simplifies `SwitchInt(_) -> [targets]`, | ||
| /// where all the `targets` have the same form, | ||
| /// into `goto -> target_first`. | ||
| pub struct SimplifyBranchSame; |
There was a problem hiding this comment.
I don't think we can do this unless there is no debuginfo attached to scopes used only in the arms, or we can convert the debuginfo into a form that switches on the SwitchInt operand (plausible in DWARF, not really exposed in LLVM).
There was a problem hiding this comment.
But also this optimization can be rephrased as sinking identical statements from predecessors into the start of a block, followed by simplify_cfg.
simplify_try: address some of eddyb's comments Addresses only rust-lang#66282 (comment) and rust-lang#66282 (comment). r? @eddyb cc @oli-obk
This PR works towards #66234 by providing two optimization passes meant to run in sequence:
SimplifyArmIdentitywhich transforms something like:into:
_LOCAL_0 = move _LOCAL_1SimplifyBranchSamewhich transformsSwitchInts to identical basic blocks into agototo the first reachable target.Together, these are meant to simplify the following into just
res:It should be noted however that the desugaring of
?includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity onx. Inlining requiresmir-opt-level=2so this might not have any effect in perf-bot but let's find out.r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)