[llvm-commits] [llvm] r164641 - in /llvm/trunk: lib/Transforms/Scalar/SROA.cpp test/Transforms/SROA/basictest.ll

[Chandler Carruth]

On Tue, Sep 25, 2012 at 7:20 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> On Tue, Sep 25, 2012 at 7:00 PM, Nick Lewycky <nlewycky at google.com> wrote:
> > On 25 September 2012 18:10, Eli Friedman <eli.friedman at gmail.com> wrote:
> >>
> >> On Tue, Sep 25, 2012 at 3:46 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> >> > Author: nicholas
> >> > Date: Tue Sep 25 17:46:21 2012
> >> > New Revision: 164641
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=164641&view=rev
> >> > Log:
> >> > Don't drop the alignment on a memcpy intrinsic when producing a store.
> >> > This is
> >> > only a missed optimization opportunity if the store is over-aligned,
> but
> >> > a
> >> > miscompile if the store's new type has a higher natural alignment than
> >> > the
> >> > memcpy did. Fixes PR13920!
> >> >
> >> > Modified:
> >> >     llvm/trunk/lib/Transforms/Scalar/SROA.cpp
> >> >     llvm/trunk/test/Transforms/SROA/basictest.ll
> >> >
> >> > Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp
> >> > URL:
> >> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=164641&r1=164640&r2=164641&view=diff
> >> >
> >> >
> ==============================================================================
> >> > --- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)
> >> > +++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Tue Sep 25 17:46:21 2012
> >> > @@ -2272,8 +2272,9 @@
> >> >                                      getName(".insert"));
> >> >      }
> >> >
> >> > -    Value *Store = IRB.CreateStore(Src, DstPtr, II.isVolatile());
> >> > -    (void)Store;
> >> > +    StoreInst *Store = cast<StoreInst>(IRB.CreateStore(Src, DstPtr,
> >> > +
> >> > II.isVolatile()));
> >> > +    Store->setAlignment(II.getAlignment());
> >>
> >> This isn't quite right: "0" alignment on a store has a different
> >> meaning from "0" alignment on a memcpy.
> >
> >
> > Well spotted, I was not aware of the difference! However, I looked at
> > LangRef and I disagree with your analysis:
> >
> > store: "A value of 0 or an omitted "align" argument means that the
> operation
> > has the preferential alignment for the target."
> >
> > memcpy: "If the call to this intrinsic has an alignment value that is
> not 0
> > or 1, then the caller guarantees that both the source and destination
> > pointers are aligned to that boundary."
> >
> > So transforming zero to zero is fine as they both mean preferred
> alignment.
>
> I'm not sure how you got here: if the alignment value is 0 or 1 for
> memcpy, the caller makes no alignment guarantees (per the section you
> quoted).
>
>
> Anyway, I just discovered there's another more serious issue with your
> commit: you aren't taking into account the offset between the original
> memcpy and the generated store.  This is causing a failure on the
> bullet benchmark from the testsuite on 32-bit x86.  I'm getting output
> like the following for stores expanded from a memcpy:
>

Yea, this is just the tip of the iceberg here. I'm sorry about this, it
slipped through my own review and my reviewers: the SROA pass completely
ignored alignment on loads and stores it is synthesizing. =/ For example,
there are loads just a bit above this that are still misaligned. I would
just disable it, but I'm most of the way through fixing this, and should
commit the fix momentarily. I'll also do a MinAlign computation here.


>
> [...]
>   %groundTransform.sroa.0.0.tmp.i.i.i.i43.i.i.idx = getelementptr
> inbounds %struct.btDefaultMotionState* %tmp53, i32 0, i32 1, i32 0,
> i32 0, i32 0, i32 0, i32 0, i32 0
>   store float 1.000000e+00, float*
> %groundTransform.sroa.0.0.tmp.i.i.i.i43.i.i.idx, align 16
>   %groundTransform.sroa.1.4.tmp.i.i.i.i43.i.i.idx120 = getelementptr
> inbounds %struct.btDefaultMotionState* %tmp53, i32 0, i32 1, i32 0,
> i32 0, i32 0, i32 0, i32 0, i32 1
>   store float 0.000000e+00, float*
> %groundTransform.sroa.1.4.tmp.i.i.i.i43.i.i.idx120, align 16
>   %groundTransform.sroa.2.8.tmp.i.i.i.i43.i.i.idx123 = getelementptr
> inbounds %struct.btDefaultMotionState* %tmp53, i32 0, i32 1, i32 0,
> i32 0, i32 0, i32 0, i32 0, i32 2
>   store float 0.000000e+00, float*
> %groundTransform.sroa.2.8.tmp.i.i.i.i43.i.i.idx123, align 16
> [...]
>
> -Eli
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120926/90b59490/attachment.html>