[llvm-commits] Struct-handling patches

[Dan Gohman]

On Sep 28, 2008, at 12:04 PM, Matthijs Kooijman wrote:

> Hi all,
>
> please find three patches attached. These patches fix some behaviour  
> related
> to struct transformations.
>
> I was observing issues with struct alloca's not always being  
> properly split up
> by scalarrepl. Some other passes sometimes make changes that confuse
> scalarrepl, preventing it from doing its work.
>
> I don't have a clear-cut testcase for this, since the issues occured  
> when
> running our own ordering of LLVM passes. Using -std-compile-opts or  
> llvm-gcc
> mostly managed to mask thes bugs by doing transformations in a  
> particular
> order. I have include small testcases with each patch, though.
>
> The first patch, preserve-struct.diff changes instcombine not to  
> replace a
> struct alloca by some other type in some cases. This is currently  
> done if the
> alloca is bitcasted to any type with a higher alignment (such as i32  
> and i64),
> which greatly confuses scalarrepl. This patch is not without  
> problems, since
> it makes the llvm-gcc bug in PR2823 happen a bit more often. For  
> more details
> about this patch, see the thread at
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2008-September/016870.html

I think this is fine. It's a tricky area, and I expect it'll be  
something
we'll re-evaluate over time, but I think it's reasonable for now.

>
>
> The second patch, vector-gep.diff, allows scalarrepl to treat all GEP
> instructions with all zero indices as bitcasts. Sometimes, all-zero  
> bitcasts
> are emitted where bitcasts would be conceptually more appropriate.  
> Currently,
> scalarrepl handles all-zero geps only when there are no vector types  
> involved.
> This patch removes this limitation.

+    if (isa<ArrayType>(*I)) {
+      // Otherwise, we must have an index into an array type.  Verify  
that this is
+      // an in-range constant integer.  Specifically, consider A[0] 
[i].  We
+      // cannot know that the user isn't doing invalid things like  
allowing i to
+      // index an out-of-range subscript that accesses A[1].  Because  
of this, we
+      // have to reject SROA of any accesses into structs where any  
of the
+      // components are variables.
+      if (IdxVal->getZExtValue() >= cast<ArrayType>(*I)- 
 >getNumElements())
+        return MarkUnsafe(Info);
+    }

Instead of isa + cast you can use
   if (const ArrayType *ATy = dyn_cast<ArrayType>(*I)) {

Also, the word "Otherwise" in the comment no longer makes sense,
given the new structure of the code.

Otherwise :-), this patch looks good.

>
>
> The last patch, firstclass.diff, concerns instcombine. Instcombine  
> can replace
> memcpy calls for small values with a load and a store. This defaults  
> to using
> an integer type of appropriate size to load and store, unless a  
> single value
> type is copied (ie, a double, or a struct containing just a double,  
> etc.). The
> patch changes this to any first class type, since all first class  
> types can be
> loaded and stored directly. In particular, a memcpy of a small  
> struct is now
> replaced by a load and a store of the right struct type, instead of  
> casting it
> to integer first (confusing scalarrepl more).

I agree with Duncan that this doesn't sound safe in the case of
structs with holes.

>
> This last patch is not completely useful by itself yet, since  
> scalarrepl
> doesn't know how to handle struct loads and stores yet :-) I'll have  
> a look at
> that tomorrow.

Would it make sense to teach scalarrepl how to handle llvm.memcpy? The  
code
above in instcombine only looks at objects that are 8 bytes or smaller,
which is somewhat arbitrary when it comes to structs. We don't want to
translate large struct copies to single loads and stores, but it seems
there will be a lot of interesting cases with structs slightly larger  
than
8 bytes.

Actually, it may some day make sense to use single loads and stores
instead of memcpy for all structs, but before we can do that we'd need
at least need to teach codegen how to effectively turn them back into
memcpy in most cases. Offhand I don't know what other changes would
be needed to avoid major pessimizations.

Dan