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