Optimize load from aggregate stores

Mon Jun 30 11:24:14 PDT 2014

ping ?!??


2014-06-26 21:32 GMT-07:00 deadal nix <deadalnix at gmail.com>:

> Hi,
>
> So I updated the patch to include more sanity checks and tests for load
> past the end of the store.
>
> CoerceAvailableValueToLoadType will return null if it doesn't know how to
> do a bitcast between the values provided, or if the load is larger than the
> store.
>
> My understanding is that
>
> @@ -1930,6 +1947,11 @@ bool GVN::processLoad(LoadInst *L) {
>
>        if (DL) {
>          StoredVal = CoerceAvailableValueToLoadType(StoredVal,
> L->getType(),
>                                                     L, *DL);
>
> This code is not calling GetStoreValueForLoad because it knows it won't
> need the bit manipulations of the more general case. This assertion is true
> when you deal with basic types, but do not hold anymore for struct/array.
> It still seems worthwhile to try the fast path as it should be the taken
> most of the time.
>
>
> 2014-06-26 4:24 GMT-07:00 Hal Finkel <hfinkel at anl.gov>:
>
> ----- 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140630/fb90cfbf/attachment.html>