[LLVMdev] alignment checking in isSafeToEliminateVarargsCast

[Eli Friedman]

On Fri, Jul 29, 2011 at 11:13 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:
> I have a question about a problem I came across while I was adding support
> for aggregate va_arg expression in clang.
> The following is the example program I will use in this email. I compile the
> program with clang targeting mips. Note that I have not pushed all the
> changes I have made yet, so you will not be able to see the same results.
>
> $ clang  -ccc-host-triple mipsel-unknown-linux -ccc-clang-archs mipsel foo.c
> -o foo.ll  -emit-llvm -O3 -S
> // foo.c
> // adapted from test-suite/SingleSource/UnitTests/2003-05-07-VarArg
> s.c
> #include <stdarg.h>
>
> typedef struct DWordS_struct    { int i; char c; } DWordS;
> typedef struct LargeS_struct { int i; double d; DWordS* ptr; int j; }
> LargeS;
>
> void test(char *fmt,...);
>
> int main() {
>   DWordS dw = { 18, 'a' };
>   LargeS ls = { 21, 22.0, &dw, 23 };
>
>   test("DQL", dw, ls);
>
>   return 0;
> }
>
> After code generation, clang runs several llvm IR optimization passes
> including InstCombine.
> The following is the bitcode before InstCombine is run. The second argument
> %dw of function test is of type "%struct.DWordS_struct* byval", which has a
> 4-byte alignment.
>
>
> Breakpoint 1 at
> llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1652
> 1652                   InstCombineIRInserter(Worklist));
> (gdb) p F.dump()
>
> define i32 @main() nounwind {
> entry:
>   %dw = alloca %struct.DWordS_struct, align 4
>   %ls = alloca %struct.LargeS_struct, align 8
>   %tmp = bitcast %struct.DWordS_struct* %dw to i8*
>   call void @llvm.memcpy.p0i8.p0i8.i32(i8* %tmp, i8* bitcast ({ i32, i8, [3
> x i8] }* @main.dw to i8*), i32 8, i32 4, i1 false)
>   %i = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 0
>   store i32 21, i32* %i, align 4, !tbaa !0
>   %d = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 1
>   store double 2.200000e+01, double* %d, align 8, !tbaa !3
>   %ptr = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 2
>   store %struct.DWordS_struct* %dw, %struct.DWordS_struct** %ptr, align 4,
> !tbaa !4
>   %j = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 3
>   store i32 23, i32* %j, align 4, !tbaa !0
>   call void (i8*, ...)* @test(i8* getelementptr inbounds ([4 x i8]* @.str,
> i32 0, i32 0), %struct.DWordS_struct* byval %dw, %struct.LargeS_struct*
> byval %ls)
>   ret i32 0
> }
>
>
> After InstCombine is run, the type of the second argument of function test
> has been changed to "i64* byval", which has an 8-byte alignment. This
> becomes a problem if the target is mips-o32, since a function call uses
> different registers or different stack locations to pass arguments depending
> on the alignment of the type of the argument that is passed.
>
> Breakpoint 2 at
> llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1667
> 1667      return EverMadeChange;
> (gdb) p F.dump()
> define i32 @main() nounwind {
> entry:
>   %dw = alloca i64, align 8
>   %tmpcast = bitcast i64* %dw to %struct.DWordS_struct*
>   %ls = alloca %struct.LargeS_struct, align 8
>   store i64 416611827730, i64* %dw, align 8
>   %i = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 0
>   store i32 21, i32* %i, align 8, !tbaa !0
>   %d = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 1
>   store double 2.200000e+01, double* %d, align 8, !tbaa !3
>   %ptr = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 2
>   store %struct.DWordS_struct* %tmpcast, %struct.DWordS_struct** %ptr, align
> 8, !tbaa !4
>   %j = getelementptr inbounds %struct.LargeS_struct* %ls, i32 0, i32 3
>   store i32 23, i32* %j, align 4, !tbaa !0
>   call void (i8*, ...)* @test(i8* getelementptr inbounds ([4 x i8]* @.str,
> i32 0, i32 0), i64* byval %dw, %struct.LargeS_struct* byval %ls) nounwind
>   ret i32 0
> }
>
> This transformation seems to take place in InstCombiner::visitCallSite in
> InstCombineCalls.cpp, which calls function isSafeToEliminateVarargsCast to
> decide whether it is safe to eliminate a cast instruction and replace the
> argument that is passed. Would it be possible to add code in
> isSafeToEliminateVarargsCast that checks alignments of SrcTy and DstTy and
> returns false if they are not the same? Or are there better ways to
> circumvent this problem?

Looks like a legitimate bug.  We really shouldn't be touching the
types of byval arguments...

-Eli