[PATCH] LLVM miscompiles function call with struct parameter for AArch64 big-endian

[John Brawn]

The patch makes a line over 0 characters, doing it instead as
    Type *TargetTy = IsSplit 
      ? Type::getIntNTy(LI.getContext(),
DL.getTypeStoreSizeInBits(NewAllocaTy))
      : LI.getType();
fixes that.

The patch looks OK to me, but I don't know enough about SROA to know if it's
correct. It looks like 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. This did make me wonder if the problem is instead the size of the
slices, but the alloca is
accessed by a 8-byte load so I think it's inevitable.

Also it looks like this would affect any big-endian target with similar data
layout and 64-bit registers,
not just AArch64.

John

> From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Alexandros Lamprineas
> Sent: 03 June 2015 17:10
> To: llvm-commits at cs.uiuc.edu
> Subject: [PATCH] LLVM miscompiles function call with struct parameter for
AArch64 big-endian
>
> The attached code is miscompiled when targeting big-endian AArch64 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 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 llvm/lib/Transforms/Scalar/SROA.cpp. The
‘Scalar Replacement Of Aggregates’ 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’.
>
> Alexandros Lamprineas