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

[Chandler Carruth]

Ok, across a few commits I've significantly improved the state of setting
up alignment properly for loads and stores. There is still work to be done
here though as there are more places where we fail to track it properly.
I'll continue looking at this, and especially the stable_sort failure as I
can.

Again, if this is impeding folks, or blocking progress, please don't stress
too much about just turning it off.


On Wed, Sep 26, 2012 at 1:12 AM, Chandler Carruth <chandlerc at google.com>wrote:

>
> 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/d3f66dd9/attachment.html>