[llvm] r225254 - InstCombine: Bitcast call arguments from/to pointer/integer type
Steven Wu
stevenwu at apple.com
Wed Mar 11 10:16:59 PDT 2015
Hi David
I found a problem introduced by this patch. We don’t respect byval attribute on the function parameter when we instcombine function cast. Attach a testcase.
In the testcase, test_byval function has a parameter defined as pass byval, but at the callsite, it does the argument expansion.
Without instcombine, they produces compatible interface. After instcombine, now test_byval treats the first field in the struct as the pointer and everything fall apart.
This optimization used to be stopped by isBitCastable, but now the data layout says it is ok to cast a pointer to i32.
Since we actually check the byval attribute on the argument, it is weird that we didn’t do the same check for parameter. If you think this is the right fix, I will prepare a patch for it.
Thanks
Steven
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testcase.ll
Type: application/octet-stream
Size: 718 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150311/4f754d46/attachment.obj>
-------------- next part --------------
> On Jan 6, 2015, at 12:41 AM, David Majnemer <david.majnemer at gmail.com> wrote:
>
> Author: majnemer
> Date: Tue Jan 6 02:41:31 2015
> New Revision: 225254
>
> URL: http://llvm.org/viewvc/llvm-project?rev=225254&view=rev
> Log:
> InstCombine: Bitcast call arguments from/to pointer/integer type
>
> Try harder to get rid of bitcast'd calls by ptrtoint/inttoptr'ing
> arguments and return values when DataLayout says it is safe to do so.
>
> Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
> llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll
> llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=225254&r1=225253&r2=225254&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue Jan 6 02:41:31 2015
> @@ -1416,7 +1416,7 @@ bool InstCombiner::transformConstExprCas
> if (NewRetTy->isStructTy())
> return false; // TODO: Handle multiple return values.
>
> - if (!CastInst::isBitCastable(NewRetTy, OldRetTy)) {
> + if (!CastInst::isBitOrNoopPointerCastable(NewRetTy, OldRetTy, DL)) {
> if (Callee->isDeclaration())
> return false; // Cannot transform this return value.
>
> @@ -1451,12 +1451,21 @@ bool InstCombiner::transformConstExprCas
> unsigned NumActualArgs = CS.arg_size();
> unsigned NumCommonArgs = std::min(FT->getNumParams(), NumActualArgs);
>
> + // Prevent us turning:
> + // declare void @takes_i32_inalloca(i32* inalloca)
> + // call void bitcast (void (i32*)* @takes_i32_inalloca to void (i32)*)(i32 0)
> + //
> + // into:
> + // call void @takes_i32_inalloca(i32* null)
> + if (Callee->getAttributes().hasAttrSomewhere(Attribute::InAlloca))
> + return false;
> +
> CallSite::arg_iterator AI = CS.arg_begin();
> for (unsigned i = 0, e = NumCommonArgs; i != e; ++i, ++AI) {
> Type *ParamTy = FT->getParamType(i);
> Type *ActTy = (*AI)->getType();
>
> - if (!CastInst::isBitCastable(ActTy, ParamTy))
> + if (!CastInst::isBitOrNoopPointerCastable(ActTy, ParamTy, DL))
> return false; // Cannot transform this parameter value.
>
> if (AttrBuilder(CallerPAL.getParamAttributes(i + 1), i + 1).
> @@ -1551,7 +1560,7 @@ bool InstCombiner::transformConstExprCas
> if ((*AI)->getType() == ParamTy) {
> Args.push_back(*AI);
> } else {
> - Args.push_back(Builder->CreateBitCast(*AI, ParamTy));
> + Args.push_back(Builder->CreateBitOrPointerCast(*AI, ParamTy));
> }
>
> // Add any parameter attributes.
> @@ -1622,7 +1631,7 @@ bool InstCombiner::transformConstExprCas
> Value *NV = NC;
> if (OldRetTy != NV->getType() && !Caller->use_empty()) {
> if (!NV->getType()->isVoidTy()) {
> - NV = NC = CastInst::Create(CastInst::BitCast, NC, OldRetTy);
> + NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);
> NC->setDebugLoc(Caller->getDebugLoc());
>
> // If this is an invoke instruction, we should insert it after the first
>
> Modified: llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll Tue Jan 6 02:41:31 2015
> @@ -5,15 +5,18 @@ target triple = "i686-pc-linux-gnu"
>
> define i32 @main() {
> ; CHECK-LABEL: @main(
> -; CHECK: call i32 bitcast
> +; CHECK: %[[call:.*]] = call i7* @ctime(i999* null)
> +; CHECK: %[[cast:.*]] = ptrtoint i7* %[[call]] to i32
> +; CHECK: ret i32 %[[cast]]
> entry:
> %tmp = call i32 bitcast (i7* (i999*)* @ctime to i32 (i99*)*)( i99* null )
> ret i32 %tmp
> }
>
> define i7* @ctime(i999*) {
> -; CHECK-LABEL: @ctime(
> -; CHECK: call i7* bitcast
> +; CHECK-LABEL: define i7* @ctime(
> +; CHECK: %[[call:.*]] = call i32 @main()
> +; CHECK: %[[cast:.*]] = inttoptr i32 %[[call]] to i7*
> entry:
> %tmp = call i7* bitcast (i32 ()* @main to i7* ()*)( )
> ret i7* %tmp
>
> Modified: llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll Tue Jan 6 02:41:31 2015
> @@ -5,7 +5,9 @@ target triple = "i686-pc-linux-gnu"
>
> define i32 @main() {
> ; CHECK-LABEL: @main
> -; CHECK: call i32 bitcast
> +; CHECK: %[[call:.*]] = call i8* @ctime(i32* null)
> +; CHECK: %[[cast:.*]] = ptrtoint i8* %[[call]] to i32
> +; CHECK: ret i32 %[[cast]]
> entry:
> %tmp = call i32 bitcast (i8* (i32*)* @ctime to i32 (i32*)*)( i32* null ) ; <i32> [#uses=1]
> ret i32 %tmp
> @@ -25,3 +27,37 @@ entry:
> %0 = call { i8 } bitcast ({ i8 } (i32*)* @foo to { i8 } (i16*)*)(i16* null)
> ret void
> }
> +
> +declare i32 @fn1(i32)
> +
> +define i32 @test1(i32* %a) {
> +; CHECK-LABEL: @test1
> +; CHECK: %[[cast:.*]] = ptrtoint i32* %a to i32
> +; CHECK-NEXT: %[[call:.*]] = tail call i32 @fn1(i32 %[[cast]])
> +; CHECK-NEXT: ret i32 %[[call]]
> +entry:
> + %call = tail call i32 bitcast (i32 (i32)* @fn1 to i32 (i32*)*)(i32* %a)
> + ret i32 %call
> +}
> +
> +declare i32 @fn2(i16)
> +
> +define i32 @test2(i32* %a) {
> +; CHECK-LABEL: @test2
> +; CHECK: %[[call:.*]] = tail call i32 bitcast (i32 (i16)* @fn2 to i32 (i32*)*)(i32* %a)
> +; CHECK-NEXT: ret i32 %[[call]]
> +entry:
> + %call = tail call i32 bitcast (i32 (i16)* @fn2 to i32 (i32*)*)(i32* %a)
> + ret i32 %call
> +}
> +
> +declare i32 @fn3(i64)
> +
> +define i32 @test3(i32* %a) {
> +; CHECK-LABEL: @test3
> +; CHECK: %[[call:.*]] = tail call i32 bitcast (i32 (i64)* @fn3 to i32 (i32*)*)(i32* %a)
> +; CHECK-NEXT: ret i32 %[[call]]
> +entry:
> + %call = tail call i32 bitcast (i32 (i64)* @fn3 to i32 (i32*)*)(i32* %a)
> + ret i32 %call
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list