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

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
<https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10357
&d=AwMF-g&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmu
hMHA&m=mE8ypq3K_T2S6qsheEq0sSQuaca6KhQ_tSMWi-b4sHg&s=3OuC1pq_R3PDewQI5slb0Ik
VmRcYIpojO3cbS5jsa84&e=> 

 

Regards,
Alexandros

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150630/cf901807/attachment.html>

> 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) {
>