On Mon, Jan 5, 2015 at 1:40 AM, Pete Cooper <peter_cooper at apple.com> wrote:> Hi lx, Philip > > I've seen an instcombine which helps with this situation. It fires when > the function types on both sides of the bitcast have the same number of > operands and compatible types. It then adds bitcasts on the arguments and > removes the one on the called function. >It indeed does, InstCombiner::transformConstExprCastCall.> > I don't have IR to hand, but it would be worth passing your IR through > instcombine to see if that helps you. >The following should be a sufficiently workable example of what we would hope to transform. target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" define i32 @g(i32* %a) { entry: %call = tail call i32 bitcast (i32 (i64)* @f to i32 (i32*)*)(i32* %a) ret i32 %call } declare i32 @f(i64) define i32 @h(i64 %a) { entry: %call = tail call i32 bitcast (i32 (i32*)* @g to i32 (i64)*)(i64 %a) ret i32 %call }> > The idea of improving the inliner is also great, but you may find that > it's needed for cases other than this one if i'm right about the > instcombine. >Sadly, the combine fails because InstCombine queries CastInst::isBitCastable to determine the castable-ness of the parameter type and the argument type. It isn't bitcastable though, it's ptrtoint/inttoptr castable. The following patch opens up the optimization: --- a/lib/Transforms/InstCombine/InstCombineCalls.cpp +++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp @@ -1456,7 +1456,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { 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 +1551,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { 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. Running opt -instcombine -inline -instcombine with this patch results in: define i32 @g(i32* %a) { entry: %0 = ptrtoint i32* %a to i64 %call = tail call i32 @f(i64 %0) ret i32 %call } declare i32 @f(i64) define i32 @h(i64 %a) { entry: %call.i = tail call i32 @f(i64 %a) ret i32 %call.i }> > Thanks > Pete > > Sent from my iPhone > > On Jan 5, 2015, at 3:16 AM, Liu Xin <navy.xliu at gmail.com> wrote: > > Philip, > > I post here because I think AlwaysInliner should inline it. I want to > detect the indirect calls for Inliner, and I want to hear inputs. > > let me define indirect call first in my idea. In one single expression, > one function may be subject to bitcast more than one time. we can detect > this situation and treat it as a regular call of last function, is that > okay? > > thanks, > --lx > > > On Mon, Jan 5, 2015 at 7:32 AM, Philip Reames <listmail at philipreames.com> > wrote: > >> On 01/04/2015 12:04 AM, Liu Xin wrote: >> >>> >>> %294= callfloatbitcast (float(float, float*)* @__gpu_modff to >>> float(float, i64)*)(float%293, i64 %preg.212.addr.0) >>> >>> as you may know, some gpu backends don't support function call. we need >>> to make sure to inline all functions here. however, Inliner can not figure >>> out that this is a valid callsite in this form. actually, it is. in C >>> words, cast a function and then call should be treat as callsite, right? >>> >>> Generally, the inliner doesn't do much with indirect calls, but given >> there is no simpler canonical case here, I expect we'll have to. >> >> Its possible we might even want to define this as a direct call. I'm not >> sure what the expectations are with regards to the type of the function >> being called and the type of the callsite. I suspect a lot of code would >> get confused if getCalledFunction returned __gpu_modff with it's unbitcast >> type. That's possibly something we should fix though. >> >> We'll want to get other folks input here, but a small patch to the >> inliner to handle this case would seem reasonable to me. >> >> Philip >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150105/07d3bc01/attachment.html>
On Mon, Jan 5, 2015 at 3:09 AM, David Majnemer <david.majnemer at gmail.com> wrote:> >> The idea of improving the inliner is also great, but you may find that >> it's needed for cases other than this one if i'm right about the >> instcombine. >> > > Sadly, the combine fails because InstCombine > queries CastInst::isBitCastable to determine the castable-ness of the > parameter type and the argument type. It isn't bitcastable though, it's > ptrtoint/inttoptr castable. > > The following patch opens up the optimization: > --- a/lib/Transforms/InstCombine/InstCombineCalls.cpp > +++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp > @@ -1456,7 +1456,7 @@ bool > InstCombiner::transformConstExprCastCall(CallSite CS) { > 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 +1551,7 @@ bool > InstCombiner::transformConstExprCastCall(CallSite CS) { > 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. >This is exactly why I added those casting utilities. With sufficient testing, this patch LGTM as a way to effectively canonicalize these cases. Need to make sure this handles all the cases of ptr->int and int->ptr with too large, too small, and just right integer sizes. If all goes well, this seems like a really nice improvement. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150105/871d800d/attachment.html>
Sent from my iPhone> On Jan 5, 2015, at 5:21 AM, Chandler Carruth <chandlerc at google.com> wrote: > > > On Mon, Jan 5, 2015 at 3:09 AM, David Majnemer <david.majnemer at gmail.com> wrote: >>> >>> The idea of improving the inliner is also great, but you may find that it's needed for cases other than this one if i'm right about the instcombine. >> >> Sadly, the combine fails because InstCombine queries CastInst::isBitCastable to determine the castable-ness of the parameter type and the argument type. It isn't bitcastable though, it's ptrtoint/inttoptr castable. >> >> The following patch opens up the optimization: >> --- a/lib/Transforms/InstCombine/InstCombineCalls.cpp >> +++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp >> @@ -1456,7 +1456,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { >> 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 +1551,7 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) { >> 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. > > This is exactly why I added those casting utilities. With sufficient testing, this patch LGTM as a way to effectively canonicalize these cases. Need to make sure this handles all the cases of ptr->int and int->ptr with too large, too small, and just right integer sizes. If all goes well, this seems like a really nice improvement.Nice! Can I request we add addrspacecast to the list too. It's probably in the utilities you mention but just needs a test case for this new use of the utility. Pete -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150105/15e87581/attachment.html>
Should be done in r225254. On Mon, Jan 5, 2015 at 3:21 AM, Chandler Carruth <chandlerc at google.com> wrote:> > On Mon, Jan 5, 2015 at 3:09 AM, David Majnemer <david.majnemer at gmail.com> > wrote: > >> >>> The idea of improving the inliner is also great, but you may find that >>> it's needed for cases other than this one if i'm right about the >>> instcombine. >>> >> >> Sadly, the combine fails because InstCombine >> queries CastInst::isBitCastable to determine the castable-ness of the >> parameter type and the argument type. It isn't bitcastable though, it's >> ptrtoint/inttoptr castable. >> >> The following patch opens up the optimization: >> --- a/lib/Transforms/InstCombine/InstCombineCalls.cpp >> +++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp >> @@ -1456,7 +1456,7 @@ bool >> InstCombiner::transformConstExprCastCall(CallSite CS) { >> 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 +1551,7 @@ bool >> InstCombiner::transformConstExprCastCall(CallSite CS) { >> 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. >> > > This is exactly why I added those casting utilities. With sufficient > testing, this patch LGTM as a way to effectively canonicalize these cases. > Need to make sure this handles all the cases of ptr->int and int->ptr with > too large, too small, and just right integer sizes. If all goes well, this > seems like a really nice improvement. >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150106/8098fccc/attachment.html>