Optimize load from aggregate stores

[Hal Finkel]

----- Original Message -----
> From: "deadal nix" <deadalnix at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu, "Owen Anderson" <owen at apple.com>, "Chandler Carruth" <chandlerc at google.com>
> Sent: Wednesday, June 25, 2014 2:05:41 PM
> Subject: Re: Optimize load from aggregate stores
> 
> 
> 
> Hi Hal,
> 
> If you think some other design is more appropriate, I'd be happy to
> update the diff.
> 
> 
> 
> 2014-06-25 4:34 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
> 
> 
> Hi deadal,
> 
> + // When facing an Aggregate, we have to find the element to fetch
> from.
> + while (SrcVal->getType()->isAggregateType()) {
> + if (StructType *ST = dyn_cast<StructType>(SrcVal->getType())) {
> + const StructLayout &Layout = *DL.getStructLayout(ST);
> + unsigned i = 0;
> + for (unsigned cd = 0, e = ST->getNumElements(); cd != e; ++cd) {
> + if (Layout.getElementOffset(cd) > Offset) break;
> + i = cd;
> + }
> +
> + SrcVal = Builder.CreateExtractValue(SrcVal, i);
> + Offset -= Layout.getElementOffset(i);
> 
> What happens if Offset goes beyond the end of the struct?
> 
> 
> 
> I was trusting the alias analysis to not feed me this, but that I
> true that i should add a check, let me provide an updated diff to
> make sure we don't go off road on that one. However, I'm not sure
> what a good test case for this would look like.
> 
> 
> @@ -1919,6 +1938,11 @@ bool GVN::processLoad(LoadInst *L) {
> if (DL) {
> StoredVal = CoerceAvailableValueToLoadType(StoredVal, L->getType(),
> L, *DL);
> + if (!StoredVal) {
> + // In case of aggregate store, we may have to fallback to
> GetStoreValueForLoad.
> + StoredVal = GetStoreValueForLoad(DepSI->getValueOperand(), 0,
> L->getType(), L, *DL);
> + }
> +
> if (!StoredVal)
> return false;
> 
> I'm a bit concerned here that we're changing the logic for things
> other than the aggregate case. CoerceAvailableValueToLoadType can
> return false for all sorts of reasons.
> 
> 
> 
> As said, if you think there is a better way to do this, i'd be happy
> to update the diff.

I think the first step is to understand the other reasons why CoerceAvailableValueToLoadType returns null, and see if this new behavior is generally desirable. Do you have any thoughts on that?

 -Hal

> 
> 
> 
> Owen, while I'm also concerned about code coverage here, I see no
> technical reason to discourage this: I don't see why we should force
> all frontends to adopt the Clang method, which involves lots of
> bitcasts and memcpys. This is a much larger semantic restriction
> than "put your allocas in the entry block."
> 
> Chandler, could you please look at this? Is this something that SROA
> should be dealing with first?
> 
> -Hal
> 
> 
> 
> ----- Original Message -----
> > From: "deadal nix" < deadalnix at gmail.com >
> > To: "Owen Anderson" < owen at apple.com >
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Tuesday, June 24, 2014 6:02:14 PM
> > Subject: Re: Optimize load from aggregate stores
> > 
> > 
> > 
> > ping ?
> > 
> > 
> > 
> > 
> > 2014-06-13 13:30 GMT-07:00 deadal nix < deadalnix at gmail.com > :
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 2014-06-12 9:25 GMT-07:00 Owen Anderson < owen at apple.com > :
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > On Jun 12, 2014, at 9:02 AM, Philip Reames <
> > listmail at philipreames.com > wrote:
> > 
> > 
> > 
> > 
> > On 06/12/2014 12:28 AM, Owen Anderson wrote:
> > 
> > 
> > 
> > 
> > 
> > On Jun 11, 2014, at 11:29 PM, deadal nix < deadalnix at gmail.com >
> > wrote:
> > 
> > 
> > It is gonna improve the situation quite a lot for all frontend that
> > use aggregate loads (arguably, that is a bad practice, but that no
> > reason to stab people in the back when they do it anyway).
> > 
> > 
> > I’m not sure I agree with that statement. If we don’t think they
> > should be used, not optimizing them is a good way to discourage
> > that. More generally, I’m concerned about how we will ever get good
> > test coverage of this code path, since we don’t have any extant
> > front ends that hit it. I'm joining this discussion late, but a)
> > why
> > are aggregate loads bad practice? Loading something like a small
> > struct from memory with a single load seems reasonable.
> > 
> > 
> > They are not well supported through the code generator, and
> > introducing them would add a lot of complexity. There are better
> > solutions already in use. The only encouraged use case for first
> > class structs is as multiple return values.
> > 
> > 
> > 
> > 
> > Refusing a patches to improve support because something is not well
> > supported is the best way to ensure they are never well supported.
> > 
> > 
> > 
> > 
> 
> 
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory