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

[Eli Friedman]

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:

[...]
  %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