[MIR] Deaggregate structs to enable further optimizations #35168
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
scottcarr
force-pushed
the
deaggregation
branch
from
35c9a21 to
60d06a9
Compare
| let node_path = tcx.item_path_str(tcx.map.local_def_id(node_id)); | ||
| debug!("running on: {:?}", node_path); | ||
| // we only run when mir_opt_level > 1 | ||
| match tcx.sess.opts.debugging_opts.mir_opt_level { |
There was a problem hiding this comment.
I’d suggest adding a Pass::should_run(stuff) -> bool. We previously had no use-case for such a method, which is why it does not exist currently, but somebody (…ehem, me…) foresaw such use case and left a useful comment.
scottcarr
force-pushed
the
deaggregation
branch
2 times, most recently
from
392ce66 to
62cdbea
Compare
|
Thanks @nagisa. I took out the |
| None => { return; }, | ||
| _ => {} | ||
| }; | ||
| if let MirSource::Fn(_) = source {} else { return; } |
There was a problem hiding this comment.
can you leave a comment as to why we do this if -- iirc, otherwise this triggers in constants. Seems like this may be just a temporary limitation, if we improve our constant handling a la miri.
|
OK, so I left some nits, but it seems good other than those. r=me when the nits are addressed. One other question: can you test this using your MIR optimization testing code? |
I created issue #35186 |
| // } else { | ||
| // lhs_cast | ||
| // }; | ||
| // FIXME we cannot deaggregate enums issue: 35186 |
There was a problem hiding this comment.
Nit: "#35186" makes for easier searching, I think
|
@bors r+ |
|
📌 Commit d918c99 has been approved by |
|
⌛ Testing commit d918c99 with merge 9478922... |
|
💔 Test failed - auto-win-msvc-64-opt-no-mir |
|
I am worried that this may be a pessimization for newtype structs, because it forces them to memory. |
|
@arielb1 |
|
The problem is that an SSA-value pair/newtype constructor is a no-op at the LLVM level, while a inner-field write, like However, that looks like a complication that can be handled at the MIR trans level. |
|
It seems to me that we could imagine cases where we don't want to deaggregate, but its not completely clear what those are or if they would be handled by the deaggregator or MIR trans. I opened an issue #35259 to discuss this. |
|
Given that this won't even be enabled anyway, seems like we can continu to hack on the "when to trigger it" heuristics, so I won't have that block landing! @bors r+ |
|
📌 Commit 06acf16 has been approved by |
[MIR] Deaggregate structs to enable further optimizations
Currently, we generate MIR like:
```
tmp0 = ...;
tmp1 = ...;
tmp3 = Foo { a: ..., b: ... };
```
This PR implements "deaggregation," i.e.:
```
tmp3.0 = ...
tmp3.1 = ...
```
Currently, the code only deaggregates structs, not enums. My understanding is that we do not have MIR to set the discriminant of an enum.
Currently, we generate MIR like:
This PR implements "deaggregation," i.e.:
Currently, the code only deaggregates structs, not enums. My understanding is that we do not have MIR to set the discriminant of an enum.