<div dir="ltr">Fixed in r231948.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 11, 2015 at 10:16 AM, Steven Wu <span dir="ltr"><<a href="mailto:stevenwu@apple.com" target="_blank">stevenwu@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi David<br>
<br>
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.<br>
In the testcase, test_byval function has a parameter defined as pass byval, but at the callsite, it does the argument expansion.<br>
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.<br>
This optimization used to be stopped by isBitCastable, but now the data layout says it is ok to cast a pointer to i32.<br>
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.<br>
<br>
Thanks<br>
<span class="HOEnZb"><font color="#888888"><br>
Steven<br>
</font></span><br><br>
<br>
> On Jan 6, 2015, at 12:41 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> wrote:<br>
><br>
> Author: majnemer<br>
> Date: Tue Jan 6 02:41:31 2015<br>
> New Revision: 225254<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=225254&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=225254&view=rev</a><br>
> Log:<br>
> InstCombine: Bitcast call arguments from/to pointer/integer type<br>
><br>
> Try harder to get rid of bitcast'd calls by ptrtoint/inttoptr'ing<br>
> arguments and return values when DataLayout says it is safe to do so.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll<br>
> llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll<br>
><br>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=225254&r1=225253&r2=225254&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=225254&r1=225253&r2=225254&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue Jan 6 02:41:31 2015<br>
> @@ -1416,7 +1416,7 @@ bool InstCombiner::transformConstExprCas<br>
> if (NewRetTy->isStructTy())<br>
> return false; // TODO: Handle multiple return values.<br>
><br>
> - if (!CastInst::isBitCastable(NewRetTy, OldRetTy)) {<br>
> + if (!CastInst::isBitOrNoopPointerCastable(NewRetTy, OldRetTy, DL)) {<br>
> if (Callee->isDeclaration())<br>
> return false; // Cannot transform this return value.<br>
><br>
> @@ -1451,12 +1451,21 @@ bool InstCombiner::transformConstExprCas<br>
> unsigned NumActualArgs = CS.arg_size();<br>
> unsigned NumCommonArgs = std::min(FT->getNumParams(), NumActualArgs);<br>
><br>
> + // Prevent us turning:<br>
> + // declare void @takes_i32_inalloca(i32* inalloca)<br>
> + // call void bitcast (void (i32*)* @takes_i32_inalloca to void (i32)*)(i32 0)<br>
> + //<br>
> + // into:<br>
> + // call void @takes_i32_inalloca(i32* null)<br>
> + if (Callee->getAttributes().hasAttrSomewhere(Attribute::InAlloca))<br>
> + return false;<br>
> +<br>
> CallSite::arg_iterator AI = CS.arg_begin();<br>
> for (unsigned i = 0, e = NumCommonArgs; i != e; ++i, ++AI) {<br>
> Type *ParamTy = FT->getParamType(i);<br>
> Type *ActTy = (*AI)->getType();<br>
><br>
> - if (!CastInst::isBitCastable(ActTy, ParamTy))<br>
> + if (!CastInst::isBitOrNoopPointerCastable(ActTy, ParamTy, DL))<br>
> return false; // Cannot transform this parameter value.<br>
><br>
> if (AttrBuilder(CallerPAL.getParamAttributes(i + 1), i + 1).<br>
> @@ -1551,7 +1560,7 @@ bool InstCombiner::transformConstExprCas<br>
> if ((*AI)->getType() == ParamTy) {<br>
> Args.push_back(*AI);<br>
> } else {<br>
> - Args.push_back(Builder->CreateBitCast(*AI, ParamTy));<br>
> + Args.push_back(Builder->CreateBitOrPointerCast(*AI, ParamTy));<br>
> }<br>
><br>
> // Add any parameter attributes.<br>
> @@ -1622,7 +1631,7 @@ bool InstCombiner::transformConstExprCas<br>
> Value *NV = NC;<br>
> if (OldRetTy != NV->getType() && !Caller->use_empty()) {<br>
> if (!NV->getType()->isVoidTy()) {<br>
> - NV = NC = CastInst::Create(CastInst::BitCast, NC, OldRetTy);<br>
> + NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);<br>
> NC->setDebugLoc(Caller->getDebugLoc());<br>
><br>
> // If this is an invoke instruction, we should insert it after the first<br>
><br>
> Modified: llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll (original)<br>
> +++ llvm/trunk/test/Transforms/InstCombine/apint-call-cast-target.ll Tue Jan 6 02:41:31 2015<br>
> @@ -5,15 +5,18 @@ target triple = "i686-pc-linux-gnu"<br>
><br>
> define i32 @main() {<br>
> ; CHECK-LABEL: @main(<br>
> -; CHECK: call i32 bitcast<br>
> +; CHECK: %[[call:.*]] = call i7* @ctime(i999* null)<br>
> +; CHECK: %[[cast:.*]] = ptrtoint i7* %[[call]] to i32<br>
> +; CHECK: ret i32 %[[cast]]<br>
> entry:<br>
> %tmp = call i32 bitcast (i7* (i999*)* @ctime to i32 (i99*)*)( i99* null )<br>
> ret i32 %tmp<br>
> }<br>
><br>
> define i7* @ctime(i999*) {<br>
> -; CHECK-LABEL: @ctime(<br>
> -; CHECK: call i7* bitcast<br>
> +; CHECK-LABEL: define i7* @ctime(<br>
> +; CHECK: %[[call:.*]] = call i32 @main()<br>
> +; CHECK: %[[cast:.*]] = inttoptr i32 %[[call]] to i7*<br>
> entry:<br>
> %tmp = call i7* bitcast (i32 ()* @main to i7* ()*)( )<br>
> ret i7* %tmp<br>
><br>
> Modified: llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll?rev=225254&r1=225253&r2=225254&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll (original)<br>
> +++ llvm/trunk/test/Transforms/InstCombine/call-cast-target.ll Tue Jan 6 02:41:31 2015<br>
> @@ -5,7 +5,9 @@ target triple = "i686-pc-linux-gnu"<br>
><br>
> define i32 @main() {<br>
> ; CHECK-LABEL: @main<br>
> -; CHECK: call i32 bitcast<br>
> +; CHECK: %[[call:.*]] = call i8* @ctime(i32* null)<br>
> +; CHECK: %[[cast:.*]] = ptrtoint i8* %[[call]] to i32<br>
> +; CHECK: ret i32 %[[cast]]<br>
> entry:<br>
> %tmp = call i32 bitcast (i8* (i32*)* @ctime to i32 (i32*)*)( i32* null ) ; <i32> [#uses=1]<br>
> ret i32 %tmp<br>
> @@ -25,3 +27,37 @@ entry:<br>
> %0 = call { i8 } bitcast ({ i8 } (i32*)* @foo to { i8 } (i16*)*)(i16* null)<br>
> ret void<br>
> }<br>
> +<br>
> +declare i32 @fn1(i32)<br>
> +<br>
> +define i32 @test1(i32* %a) {<br>
> +; CHECK-LABEL: @test1<br>
> +; CHECK: %[[cast:.*]] = ptrtoint i32* %a to i32<br>
> +; CHECK-NEXT: %[[call:.*]] = tail call i32 @fn1(i32 %[[cast]])<br>
> +; CHECK-NEXT: ret i32 %[[call]]<br>
> +entry:<br>
> + %call = tail call i32 bitcast (i32 (i32)* @fn1 to i32 (i32*)*)(i32* %a)<br>
> + ret i32 %call<br>
> +}<br>
> +<br>
> +declare i32 @fn2(i16)<br>
> +<br>
> +define i32 @test2(i32* %a) {<br>
> +; CHECK-LABEL: @test2<br>
> +; CHECK: %[[call:.*]] = tail call i32 bitcast (i32 (i16)* @fn2 to i32 (i32*)*)(i32* %a)<br>
> +; CHECK-NEXT: ret i32 %[[call]]<br>
> +entry:<br>
> + %call = tail call i32 bitcast (i32 (i16)* @fn2 to i32 (i32*)*)(i32* %a)<br>
> + ret i32 %call<br>
> +}<br>
> +<br>
> +declare i32 @fn3(i64)<br>
> +<br>
> +define i32 @test3(i32* %a) {<br>
> +; CHECK-LABEL: @test3<br>
> +; CHECK: %[[call:.*]] = tail call i32 bitcast (i32 (i64)* @fn3 to i32 (i32*)*)(i32* %a)<br>
> +; CHECK-NEXT: ret i32 %[[call]]<br>
> +entry:<br>
> + %call = tail call i32 bitcast (i32 (i64)* @fn3 to i32 (i32*)*)(i32* %a)<br>
> + ret i32 %call<br>
> +}<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br></blockquote></div><br></div>