Optimize load from aggregate stores

[Hal Finkel]

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?

@@ -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.

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