Optimize load from aggregate stores

[deadal nix]

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.


> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140625/c71a96d7/attachment.html>