Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIR] when should we not deaggregate? #35259

Closed
scottcarr opened this issue Aug 3, 2016 · 6 comments
Closed

[MIR] when should we not deaggregate? #35259

scottcarr opened this issue Aug 3, 2016 · 6 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottcarr
Copy link
Contributor

scottcarr commented Aug 3, 2016

#35168 adds the capability of deaggregating structs like:

tmp0 = ...;
tmp1 = ...;
tmp2 = Foo { a: ..., b: ... };

Into:

tmp2.0 = ...
tmp2.1 = ...

But, one could imagine situations where this is counter productive.

It's not clear if this should be handled by the Deaggregator or MIR trans.

@eddyb eddyb added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Aug 3, 2016
@nagisa
Copy link
Member

nagisa commented Aug 4, 2016

I can’t think of a trivial way to reverse deaggregation, especially after multiple optimisation passes go through the MIR.

The newtype concern raised by Ariel could be easily handled by replacing

tmpx: Newtype(T1, T2, ...);
tmpx.0 = ...;
tmpx.1 = ...;

with

tmpx_1: T1;
tmpx_2: T2; 
tmpx_1 = ...;
tmpx_2 = ...;

(This is probably the same thing as SRoA, which @eddyb mentioned) This likely would allow us to discard some of the Pair stuff in the translator as well, but would need a good heuristic to not become a pessimisation itself.

@nagisa
Copy link
Member

nagisa commented Aug 4, 2016

That being said, I feel like LLVM should be more than capable to handle such case quite quickly by itself.

@eddyb
Copy link
Member

eddyb commented Aug 4, 2016

@nagisa There's 2 reasons SRoA isn't enough to replace Pair: fat pointers use Pair and CheckedBinOp produces one. And yes, LLVM can handle most of these by itself anyway.

Newtypes (including those with extra ZST fields) are more relevant because of "zero-cost abstractions".

@Aatch
Copy link
Contributor

Aatch commented Aug 4, 2016

@nagisa I think the issue is about not deaggregating in some cases, rather than trying to reverse it.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 25, 2017
@eddyb
Copy link
Member

eddyb commented Feb 7, 2018

I have a prototype of optimizing through fields of locals and it's much more straight-forward to do it after deaggregation than trying to understand the creation of aggregates as a whole.
Feel free to reopen this if anything changes, but we're more likely to remove Aggregate from MIR.
(which we can do in a borrowck-friendly way via SetDiscriminant(x, 0) on all aggregates)

@eddyb eddyb closed this as completed Feb 7, 2018
@oli-obk oli-obk reopened this Jan 25, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jul 15, 2023

#107267 fixed this, right?

@oli-obk oli-obk closed this as completed Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants