[LLVMdev] [PATCH] SROA produces miscompiled code for bitfield access on big-endian targets

[Duncan P. N. Exon Smith]

> On 2015-Jun-30, at 02:14, Alexandros Lamprineas <alexandros.lamprineas at arm.com> wrote:
> 
> Hi, could you please review my patch or recommend someone else? I have pinged the CC a couple of times already.  http://reviews.llvm.org/D10357
>  
> Regards,
> Alexandros

Thanks for working on this.

I don't see a patch attached to the email here, and my mailer didn't
associate this with an ongoing mailing list thread.  I just did a search
for threads with similar subjects and I think I found the right ones.

I don't know whether this is a Phab fail or what, but typically you
should ping the same email thread where your description is, and
reattach the patch when you ping.  (It's possible that if you click the
right buttons in Phab that just happens automatically, but if you can't
find the magic just reply over email.)

Anyway, I think I found the right mail.  I've included it inline below
along with the patch that came with it.

> Hi chandlerc,
> 
> {F556279}
> 
> The attached code

What attached code?  I didn't find it in any of the threads with this
title, but I could have missed it.

> is miscompiled when targeting big-endian at all optimisation levels except for -O0. This should print "checksum = 00000008", but actually prints "checksum = 00000000". It is correctly compiled if I change the statement just before the function call to func_13 from l_15.f0 to l_15.f1 (the result of this expression is unused). The only change this causes in the IR is to change the parameter in the call to func_13 from 0x800000180 to 0x800018000.
> 
> The problem seems to be in the 'scalar replacement of aggregates' pass. The problem arises because we have a 7-byte type but the alloca is 8 bytes (because it's 4-byte aligned), which causes the aggregate to be split up into two 4-byte slices except one actually ends up being 3 bytes. The pass takes into account endianness, thus adds a shift instruction when inserting an integer to an alloca store:
>  if(DL.isBigEndian())
>    ShAmt = 8 * (DL.getTypeStoreSize(IntTy) - DL.getTypeStoreSize(Ty) - Offset);
> In this particular example an integer value of wrong size ({i32}) is passed as parameter to the function that computes the shift amount (‘insertInteger’). This causes a zero shift amount since IntTy = {i64}, Ty = {i32}, and offset = 4 bytes. My patch passes a parameter of Ty = {i24} to ‘insertInteger’.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D10357.27436.patch
Type: application/octet-stream
Size: 2564 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150630/47f99977/attachment.obj>
-------------- next part --------------


> Index: lib/Transforms/Scalar/SROA.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SROA.cpp
> +++ lib/Transforms/Scalar/SROA.cpp
> @@ -2583,8 +2583,9 @@
>      Value *OldOp = LI.getOperand(0);
>      assert(OldOp == OldPtr);
>  
> -    Type *TargetTy = IsSplit ? Type::getIntNTy(LI.getContext(), SliceSize * 8)
> -                             : LI.getType();
> +    Type *TargetTy = IsSplit ? 
> +                     Type::getIntNTy(LI.getContext(),DL.getTypeStoreSizeInBits(NewAllocaTy)) 
> +                   : LI.getType();

Can you explain why you didn't need a similar correction in
`visitStoreInst()`, which also uses `SliceSize * 8`?

Also, this patch violates the 80-column rule.  Please check the coding
standards below (I strongly recommend using clang-format, since it
handles all the whitespace for you).

http://llvm.org/docs/CodingStandards.html#source-code-formatting
http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

>      bool IsPtrAdjusted = false;
>      Value *V;
>      if (VecTy) {
>