[PATCH] Fix SROA regression causing data corruption, fixes PR19250

[Duncan P. N. Exon Smith]

> On 2014 Jun 9, at 23:39, Björn Steinbrink <bsteinbr at gmail.com> wrote:
>>> http://reviews.llvm.org/D3297
>>> 
>>> Files:
>>> lib/Transforms/Scalar/SROA.cpp
>>> test/Transforms/SROA/basictest.ll
>>> 
>>> Index: lib/Transforms/Scalar/SROA.cpp
>>> ===================================================================
>>> --- lib/Transforms/Scalar/SROA.cpp
>>> +++ lib/Transforms/Scalar/SROA.cpp
>>> @@ -1031,11 +1031,6 @@
>>>      UserTy = SI->getValueOperand()->getType();
>>>    }
>>> 
>>> -    if (!UserTy || (Ty && Ty != UserTy))
>>> -      TyIsCommon = false; // Give up on anything but an iN type.
>> 
>> I feel like this check (and assignment to `false`) should still be
>> *here*, before the early `continue`.  Can you explain why you moved it?
> 
> Because moving only the "else" part would make the results depend on the
> order of slices, which is something the code explicitly tries to avoid
> by always looking at all slices. Moving the "if" block also restores the
> behaviour from before r199771 WRT usage of UserTy vs. ITy.
> 
> For example:
> 
> define i32 @test26({ i16*, i32 }*) {
> entry-block:
>  %arg = alloca { i16*, i32 }, align 8
>  %1 = bitcast { i16*, i32 }* %0 to i8*
>  %2 = bitcast { i16*, i32 }* %arg to i8*
>  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %2, i8* %1, i32 16, i32 8, i1 false)
>  %b = getelementptr inbounds { i16*, i32 }* %arg, i64 0, i32 0
>  %pb0 = bitcast i16** %b to i8**
>  %b0 = load i8** %pb0
>  %pb1 = bitcast i16** %b to i63*
>  %b1 = load i63* %pb1
>  ret i32 0
> }
> 
> With only the "else" part moved after the early continue, this will
> have TyIsCommon set to false and return null, thus the new alloca will
> have type i16*. But if you move %pb0 and %b0 to appear after %b1, then
> TyIsCommon will be true and i8* is returned, which will then be used as
> the type for the new alloca.
> 
> With the whole if-else block moved, this consistently returns i8*,
> regardless of order.

This is what I was missing.  Thanks for the explanation.  Can you add
tests for these two cases as well?

> Another approach would be to set TyIsCommon to false when the early
> continue is taken. If that is preferable, I'll update the patch to do
> that instead.

No, I think the way you did it is more clear.  If you can think of a
good place for a comment it might help me next time I look at this.

>>> +define i32 @test25({ i1, i32 }*) {
>> 
>> I think you should create a new file for this testcase.  Name it after
>> the problem that's being fixed, and give the function a good name too.
> 
> Will do once I get feedback on the above.