Hi Manman, I'm a lurker on cfe-commits but I saw your patch, and, in addition to the issues brought up by Richard, I was wondering if you considered the case of multiple inheritance: for example, if I understand correctly, the constructors of a non-polymorphic derived class with multiple bases and a trivial constructor body would usually have a call to a non-primary base constructor as the last instruction before returning. (You might be aware of the that issue already, since it's more or less a special case of the issue Richard brought up, but just wanted to make sure.)
Best, Stephen On Fri, Mar 15, 2013 at 11:12 PM, Manman Ren <[email protected]> wrote: > > Thanks for the review. > I will look into the issues. > > Manman > > On Mar 15, 2013, at 5:28 PM, Richard Smith <[email protected]> wrote: > > On Fri, Mar 15, 2013 at 5:11 PM, Manman Ren <[email protected]> wrote: > >> Author: mren >> Date: Fri Mar 15 19:11:09 2013 >> New Revision: 177211 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=177211&view=rev >> Log: >> Exploit this-return of a callsite in a this-return function. >> >> For constructors/desctructors that return 'this', if there exists a >> callsite >> that returns 'this' and is immediately before the return instruction, make >> sure we are using the return value from the callsite. >> >> We don't need to keep 'this' alive through the callsite. It also enables >> optimizations in the backend, such as tail call optimization. >> >> rdar://12818789 >> >> Modified: >> cfe/trunk/lib/CodeGen/CGCXXABI.h >> cfe/trunk/lib/CodeGen/CGCall.cpp >> cfe/trunk/lib/CodeGen/CGClass.cpp >> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp >> cfe/trunk/lib/CodeGen/CodeGenFunction.h >> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp >> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp >> cfe/trunk/test/CodeGenCXX/arm.cpp >> >> Modified: cfe/trunk/lib/CodeGen/CGCXXABI.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.h?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGCXXABI.h (original) >> +++ cfe/trunk/lib/CodeGen/CGCXXABI.h Fri Mar 15 19:11:09 2013 >> @@ -91,6 +91,10 @@ public: >> return *MangleCtx; >> } >> >> + /// Returns true if the given instance method is one of the >> + /// kinds that the ABI says returns 'this'. >> + virtual bool HasThisReturn(GlobalDecl GD) const { return false; } >> + >> /// Find the LLVM type used to represent the given member pointer >> /// type. >> virtual llvm::Type * >> @@ -209,7 +213,8 @@ public: >> /// Emit the ABI-specific prolog for the function. >> virtual void EmitInstanceFunctionProlog(CodeGenFunction &CGF) = 0; >> >> - virtual void EmitConstructorCall(CodeGenFunction &CGF, >> + /// Emit the constructor call. Return the function that is called. >> + virtual llvm::Value *EmitConstructorCall(CodeGenFunction &CGF, >> const CXXConstructorDecl *D, >> CXXCtorType Type, bool ForVirtualBase, >> bool Delegating, >> >> Modified: cfe/trunk/lib/CodeGen/CGCall.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Fri Mar 15 19:11:09 2013 >> @@ -1705,6 +1705,18 @@ void CodeGenFunction::EmitFunctionEpilog >> llvm_unreachable("Invalid ABI kind for return argument"); >> } >> >> + // If this function returns 'this' and the last instruction is a >> CallInst >> + // that returns 'this', use the return value from the CallInst. We >> will not >> + // need to keep 'this' alive through the callsite. It also enables >> + // optimizations in the backend, such as tail call optimization. >> + if (CalleeWithThisReturn && CGM.getCXXABI().HasThisReturn(CurGD)) { >> + llvm::BasicBlock *IP = Builder.GetInsertBlock(); >> + llvm::CallInst *Callsite; >> + if (!IP->empty() && (Callsite = >> dyn_cast<llvm::CallInst>(&IP->back())) && >> + Callsite->getCalledFunction() == CalleeWithThisReturn) >> > > This doesn't look right. Suppose we have: > > struct A { A(); /* trivial dtor */ }; > struct B : A { > B() : A() { A(); } > }; > > It looks like we'll return the 'this' from the temporary. Why not compare > the llvm::Value* corresponding to the CallInst itself, rather than > comparing the callee function? (Or maybe check that the 'this' argument to > the callee matches the caller's 'this'? That'd also solve the problems > below.) > > >> + // Create a bitcast of Callsite. >> + RV = Builder.CreateBitCast(Callsite, RetAI.getCoerceToType()); >> + } >> llvm::Instruction *Ret = RV ? Builder.CreateRet(RV) : >> Builder.CreateRetVoid(); >> if (!RetDbgLoc.isUnknown()) >> Ret->setDebugLoc(RetDbgLoc); >> >> Modified: cfe/trunk/lib/CodeGen/CGClass.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGClass.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CGClass.cpp Fri Mar 15 19:11:09 2013 >> @@ -1666,8 +1666,11 @@ CodeGenFunction::EmitCXXConstructorCall( >> } >> >> // Non-trivial constructors are handled in an ABI-specific manner. >> - CGM.getCXXABI().EmitConstructorCall(*this, D, Type, ForVirtualBase, >> - Delegating, This, ArgBeg, ArgEnd); >> + llvm::Value *Callee = CGM.getCXXABI().EmitConstructorCall(*this, D, >> Type, >> + ForVirtualBase, Delegating, This, ArgBeg, >> ArgEnd); >> + if (CGM.getCXXABI().HasThisReturn(CurGD) && >> + CGM.getCXXABI().HasThisReturn(GlobalDecl(D, Type))) >> + CalleeWithThisReturn = Callee; >> > > There's no guarantee that the caller and the callee are constructing the > same object here. > > >> } >> >> void >> @@ -1756,9 +1759,12 @@ CodeGenFunction::EmitDelegateCXXConstruc >> EmitDelegateCallArg(DelegateArgs, param); >> } >> >> + llvm::Value *Callee = CGM.GetAddrOfCXXConstructor(Ctor, CtorType); >> EmitCall(CGM.getTypes().arrangeCXXConstructorDeclaration(Ctor, >> CtorType), >> - CGM.GetAddrOfCXXConstructor(Ctor, CtorType), >> - ReturnValueSlot(), DelegateArgs, Ctor); >> + Callee, ReturnValueSlot(), DelegateArgs, Ctor); >> + if (CGM.getCXXABI().HasThisReturn(CurGD) && >> + CGM.getCXXABI().HasThisReturn(GlobalDecl(Ctor, CtorType))) >> + CalleeWithThisReturn = Callee; >> } >> >> namespace { >> @@ -1825,6 +1831,9 @@ void CodeGenFunction::EmitCXXDestructorC >> EmitCXXMemberCall(DD, SourceLocation(), Callee, ReturnValueSlot(), >> This, >> VTT, >> getContext().getPointerType(getContext().VoidPtrTy), >> 0, 0); >> + if (CGM.getCXXABI().HasThisReturn(CurGD) && >> + CGM.getCXXABI().HasThisReturn(GlobalDecl(DD, Type))) >> + CalleeWithThisReturn = Callee; >> > > There's no guarantee that the caller and callee are destroying the same > object here. > > >> } >> >> namespace { >> >> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Fri Mar 15 19:11:09 2013 >> @@ -564,6 +564,9 @@ void CodeGenFunction::GenerateCode(Globa >> SourceRange BodyRange; >> if (Stmt *Body = FD->getBody()) BodyRange = Body->getSourceRange(); >> >> + // Reset CalleeWithThisReturn. >> > > Well, obviously. It would be better to explain why this is being done, not > what is being done. > > >> + CalleeWithThisReturn = 0; >> + >> // Emit the standard function prologue. >> StartFunction(GD, ResTy, Fn, FnInfo, Args, BodyRange.getBegin()); >> >> @@ -615,6 +618,8 @@ void CodeGenFunction::GenerateCode(Globa >> >> // Emit the standard function epilogue. >> FinishFunction(BodyRange.getEnd()); >> + // Reset CalleeWithThisReturn. >> + CalleeWithThisReturn = 0; >> >> // If we haven't marked the function nothrow through other means, do >> // a quick pass now to see if we can. >> >> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original) >> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Fri Mar 15 19:11:09 2013 >> @@ -1131,6 +1131,10 @@ private: >> CGDebugInfo *DebugInfo; >> bool DisableDebugInfo; >> >> + /// If the current function returns 'this', use the field to keep >> track of >> + /// the callee that returns 'this'. >> + llvm::Value *CalleeWithThisReturn; >> + >> /// DidCallStackSave - Whether llvm.stacksave has been called. Used to >> avoid >> /// calling llvm.stacksave for multiple VLAs in the same scope. >> bool DidCallStackSave; >> >> Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original) >> +++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Fri Mar 15 19:11:09 2013 >> @@ -112,7 +112,7 @@ public: >> >> void EmitInstanceFunctionProlog(CodeGenFunction &CGF); >> >> - void EmitConstructorCall(CodeGenFunction &CGF, >> + llvm::Value *EmitConstructorCall(CodeGenFunction &CGF, >> const CXXConstructorDecl *D, >> CXXCtorType Type, bool ForVirtualBase, >> bool Delegating, >> @@ -177,11 +177,11 @@ public: >> llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, llvm::Value >> *allocPtr, >> CharUnits cookieSize); >> >> -private: >> /// \brief Returns true if the given instance method is one of the >> /// kinds that the ARM ABI says returns 'this'. >> - static bool HasThisReturn(GlobalDecl GD) { >> - const CXXMethodDecl *MD = cast<CXXMethodDecl>(GD.getDecl()); >> + bool HasThisReturn(GlobalDecl GD) const { >> + const CXXMethodDecl *MD = >> dyn_cast_or_null<CXXMethodDecl>(GD.getDecl()); >> + if (!MD) return false; >> return ((isa<CXXDestructorDecl>(MD) && GD.getDtorType() != >> Dtor_Deleting) || >> (isa<CXXConstructorDecl>(MD))); >> } >> @@ -834,7 +834,7 @@ void ARMCXXABI::EmitInstanceFunctionProl >> CGF.Builder.CreateStore(getThisValue(CGF), CGF.ReturnValue); >> } >> >> -void ItaniumCXXABI::EmitConstructorCall(CodeGenFunction &CGF, >> +llvm::Value *ItaniumCXXABI::EmitConstructorCall(CodeGenFunction &CGF, >> const CXXConstructorDecl *D, >> CXXCtorType Type, bool >> ForVirtualBase, >> bool Delegating, >> @@ -849,6 +849,7 @@ void ItaniumCXXABI::EmitConstructorCall( >> // FIXME: Provide a source location here. >> CGF.EmitCXXMemberCall(D, SourceLocation(), Callee, ReturnValueSlot(), >> This, >> VTT, VTTTy, ArgBeg, ArgEnd); >> + return Callee; >> } >> >> RValue ItaniumCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF, >> >> Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original) >> +++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Fri Mar 15 19:11:09 2013 >> @@ -55,7 +55,7 @@ public: >> >> void EmitInstanceFunctionProlog(CodeGenFunction &CGF); >> >> - void EmitConstructorCall(CodeGenFunction &CGF, >> + llvm::Value *EmitConstructorCall(CodeGenFunction &CGF, >> const CXXConstructorDecl *D, >> CXXCtorType Type, bool ForVirtualBase, >> bool Delegating, >> @@ -238,7 +238,7 @@ void MicrosoftCXXABI::EmitInstanceFuncti >> } >> } >> >> -void MicrosoftCXXABI::EmitConstructorCall(CodeGenFunction &CGF, >> +llvm::Value *MicrosoftCXXABI::EmitConstructorCall(CodeGenFunction &CGF, >> const CXXConstructorDecl *D, >> CXXCtorType Type, bool >> ForVirtualBase, >> bool Delegating, >> @@ -259,6 +259,7 @@ void MicrosoftCXXABI::EmitConstructorCal >> CGF.EmitCXXMemberCall(D, SourceLocation(), Callee, ReturnValueSlot(), >> This, >> ImplicitParam, ImplicitParamTy, >> ArgBeg, ArgEnd); >> + return Callee; >> } >> >> RValue MicrosoftCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF, >> >> Modified: cfe/trunk/test/CodeGenCXX/arm.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/arm.cpp?rev=177211&r1=177210&r2=177211&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/CodeGenCXX/arm.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/arm.cpp Fri Mar 15 19:11:09 2013 >> @@ -56,15 +56,15 @@ namespace test1 { >> // CHECK: [[THIS:%.*]] = alloca [[A]]*, align 4 >> // CHECK: store [[A]]* {{.*}}, [[A]]** [[THIS]] >> // CHECK: [[THIS1:%.*]] = load [[A]]** [[THIS]] >> - // CHECK: call [[A]]* @_ZN5test11AC2Ei( >> - // CHECK: ret [[A]]* [[THIS1]] >> + // CHECK: [[THIS2:%.*]] = call [[A]]* @_ZN5test11AC2Ei( >> + // CHECK: ret [[A]]* [[THIS2]] >> >> // CHECK: define linkonce_odr [[A]]* @_ZN5test11AD1Ev([[A]]* %this) >> unnamed_addr >> // CHECK: [[THIS:%.*]] = alloca [[A]]*, align 4 >> // CHECK: store [[A]]* {{.*}}, [[A]]** [[THIS]] >> // CHECK: [[THIS1:%.*]] = load [[A]]** [[THIS]] >> - // CHECK: call [[A]]* @_ZN5test11AD2Ev( >> - // CHECK: ret [[A]]* [[THIS1]] >> + // CHECK: [[THIS2:%.*]] = call [[A]]* @_ZN5test11AD2Ev( >> + // CHECK: ret [[A]]* [[THIS2]] >> } >> >> // Awkward virtual cases. >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
