[PATCH] D12269: Add a pass to lift aggregate into allocas so SROA can get rid of them.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

In http://reviews.llvm.org/D12269#257155, @deadalnix wrote:

> In http://reviews.llvm.org/D12269#256309, @chandlerc wrote:
>
> > I indicated previously that I wasn't sure this is the correct approach. I wanted to clarify that.
> >
> > The problem I see with the approach of lowering things as loads and stores of (perhaps excessively large) integers is that I think it creates a bad canonicalization problem. Let's consider what happens when we lower as i128 loads and stores of an aggregate. Later on, we may inline and end up access the low and high 64 bits as f64 types. But in order to do that, we would have to use shift and trunc instructions on the integer values. As a consequence of these shift and truncs, we would have integer operations on the bits and avoid properly canonicalizing loads and stores or the basic domain used. =/ I'm moderately worried about this.
>
>
> You seems to be operating under the previous proposal, aka promoting aggregate as large integer stores/loads. This is a new proposal, that transform the load/store into memcpy to allocs, and let SROA do its job from there. This way of doing things would avoid the problem you mention.


Sorry that I replied to the prior proposal. Anyways, this is... interesting.

So this *only* handles loads and stores of globals? Are those common? They aren't the common case of aggregate loads and stores that I have seen...

> 

> 

> > The only remaining reason why aggregates really exist are to support return types. I actually think that's OK, they're not a terrible syntax and mechanism for modeling multiple return types. I would particularly like it if the only thing you could reasonably do is extractvalue the independent values. This would make aggregates just the LLVM mechanism for doing multiple return types with proper SSA form. I think that's fine. Maybe some day we would even want to replace it with TokenType, for now aggregates provide a nice layer of type checking and sanity checking for returns.

> 

> 

> I do not think this is the only reason. Aggregate have memory layout that depend on various target characteristics and require some logic to compute offset and alignment of various fields in them. Duplicating that logic in every frontends seems like a net lose to me.


I guess we disagree then, but we don't need to debate that on this thread.

> I'm not sure what you mean by TokenType, but in the current state of affair, it seems like we should support aggregate operations, at least.

> 

> > The second, and perhaps overriding reason why I think all of the arguments line up a different way for bitfields is that bitfields quite fundamentally *are* integers. We lose nothing by modeling them as such and create no canonicalization problems.

> 

> 

> I do think this proposal, that load from allocas, do not suffer from this problem.


Sure, this doesn't make anything worse. Again, sorry for missing the change in design, the threads have become fairly fragmented.

But I don't really understand what this is trying to improve so it is hard for me to evaluate it. It seems really silly to memcpy into allocas just so that SROA can split them and promote them again. That is a lot of compile time and effort for something that we already know how to handle.

If *all* you want to handle are loads and stores of aggregates from globals, you could take the FCA splitter that is already used in SROA and promote it to a separate pass and run it over non-alloca instructions.... This would work today AFAICT? (You would need to make it a utility as SROA would also want to run it internally.)


http://reviews.llvm.org/D12269