Reverted in r126886.<br><br>Will try to come up with a better patch.<br><br>Regards,<br><br>Tilmann<br><br><div class="gmail_quote">On Wed, Mar 2, 2011 at 10:01 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im"><br>
On Mar 2, 2011, at 11:36 AM, Tilmann Scheller wrote:<br>
<br>
> Author: tilmann<br>
> Date: Wed Mar 2 13:36:23 2011<br>
> New Revision: 126863<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=126863&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=126863&view=rev</a><br>
> Log:<br>
> Add CC_Win64ThisCall and set it in the necessary places.<br>
<br>
</div>Some comments below.<br>
<div><div></div><div class="h5"><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=126863&r1=126862&r2=126863&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=126863&r1=126862&r2=126863&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)<br>
> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Wed Mar 2 13:36:23 2011<br>
> @@ -36,6 +36,7 @@<br>
> case CC_X86StdCall: return llvm::CallingConv::X86_StdCall;<br>
> case CC_X86FastCall: return llvm::CallingConv::X86_FastCall;<br>
> case CC_X86ThisCall: return llvm::CallingConv::X86_ThisCall;<br>
> + case CC_Win64ThisCall: return llvm::CallingConv::Win64_ThisCall;<br>
> // TODO: add support for CC_X86Pascal to llvm<br>
> }<br>
> }<br>
> @@ -75,19 +76,23 @@<br>
> static const CGFunctionInfo &getFunctionInfo(CodeGenTypes &CGT,<br>
> llvm::SmallVectorImpl<CanQualType> &ArgTys,<br>
> CanQual<FunctionProtoType> FTP,<br>
> + CallingConv CC,<br>
> bool IsRecursive = false) {<br>
> // FIXME: Kill copy.<br>
> for (unsigned i = 0, e = FTP->getNumArgs(); i != e; ++i)<br>
> ArgTys.push_back(FTP->getArgType(i));<br>
> CanQualType ResTy = FTP->getResultType().getUnqualifiedType();<br>
> - return CGT.getFunctionInfo(ResTy, ArgTys, FTP->getExtInfo(), IsRecursive);<br>
> +<br>
> + return CGT.getFunctionInfo(ResTy, ArgTys,<br>
> + FTP->getExtInfo().withCallingConv(CC),<br>
> + IsRecursive);<br>
> }<br>
><br>
> const CGFunctionInfo &<br>
> CodeGenTypes::getFunctionInfo(CanQual<FunctionProtoType> FTP,<br>
> bool IsRecursive) {<br>
> llvm::SmallVector<CanQualType, 16> ArgTys;<br>
> - return ::getFunctionInfo(*this, ArgTys, FTP, IsRecursive);<br>
> + return ::getFunctionInfo(*this, ArgTys, FTP, CC_Default, IsRecursive);<br>
> }<br>
><br>
> static CallingConv getCallingConventionForDecl(const Decl *D) {<br>
> @@ -114,8 +119,10 @@<br>
> // Add the 'this' pointer.<br>
> ArgTys.push_back(GetThisType(Context, RD));<br>
><br>
> + CallingConv CC = Context.Target.isWin64() ? CC_Win64ThisCall : CC_Default;<br>
> +<br>
<br>
</div></div>This kind of thing makes me very nervous. Why isn't the calling convention already part of the function type when we get here? And, if there is a calling convention specified on the function type (e.g., through an attribute), won't we end up ignoring it?<br>
<div class="im"><br>
> return ::getFunctionInfo(*this, ArgTys,<br>
> - FTP->getCanonicalTypeUnqualified().getAs<FunctionProtoType>());<br>
> + FTP->getCanonicalTypeUnqualified().getAs<FunctionProtoType>(), CC);<br>
> }<br>
><br>
> const CGFunctionInfo &CodeGenTypes::getFunctionInfo(const CXXMethodDecl *MD) {<br>
> @@ -128,7 +135,9 @@<br>
> if (MD->isInstance())<br>
> ArgTys.push_back(GetThisType(Context, MD->getParent()));<br>
><br>
> - return ::getFunctionInfo(*this, ArgTys, GetFormalType(MD));<br>
> + CallingConv CC = Context.Target.isWin64() ? CC_Win64ThisCall : CC_Default;<br>
> +<br>
> + return ::getFunctionInfo(*this, ArgTys, GetFormalType(MD), CC);<br>
> }<br>
<br>
</div>Same questions/concerns as above.<br>
<br>
Also, do static member functions really use CC_Win64ThisCall, even when there is no "this"?<br>
<div class="im"><br>
> const CGFunctionInfo &CodeGenTypes::getFunctionInfo(const CXXConstructorDecl *D,<br>
> @@ -145,7 +154,9 @@<br>
> for (unsigned i = 0, e = FTP->getNumArgs(); i != e; ++i)<br>
> ArgTys.push_back(FTP->getArgType(i));<br>
><br>
> - return getFunctionInfo(ResTy, ArgTys, FTP->getExtInfo());<br>
> + CallingConv CC = Context.Target.isWin64() ? CC_Win64ThisCall : CC_Default;<br>
> +<br>
> + return getFunctionInfo(ResTy, ArgTys, FTP->getExtInfo().withCallingConv(CC));<br>
> }<br>
<br>
</div>Again :)<br>
<div class="im"><br>
> const CGFunctionInfo &CodeGenTypes::getFunctionInfo(const CXXDestructorDecl *D,<br>
> @@ -159,7 +170,9 @@<br>
> CanQual<FunctionProtoType> FTP = GetFormalType(D);<br>
> assert(FTP->getNumArgs() == 0 && "dtor with formal parameters");<br>
><br>
> - return getFunctionInfo(ResTy, ArgTys, FTP->getExtInfo());<br>
> + CallingConv CC = Context.Target.isWin64() ? CC_Win64ThisCall : CC_Default;<br>
> +<br>
> + return getFunctionInfo(ResTy, ArgTys, FTP->getExtInfo().withCallingConv(CC));<br>
> }<br>
<br>
</div>Again :)<br>
<div><div></div><div class="h5"><br>
> const CGFunctionInfo &CodeGenTypes::getFunctionInfo(const FunctionDecl *FD) {<br>
> @@ -250,8 +263,8 @@<br>
> return *FI;<br>
><br>
> // Construct the function info.<br>
> - FI = new CGFunctionInfo(CC, Info.getNoReturn(), Info.getRegParm(), ResTy,<br>
> - ArgTys.data(), ArgTys.size());<br>
> + FI = new CGFunctionInfo(CC, Info.getNoReturn(), Info.getRegParm(),<br>
> + ResTy, ArgTys.data(), ArgTys.size());<br>
> FunctionInfos.InsertNode(FI, InsertPos);<br>
><br>
> // Compute ABI information.<br>
><br>
> Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=126863&r1=126862&r2=126863&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=126863&r1=126862&r2=126863&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp (original)<br>
> +++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp Wed Mar 2 13:36:23 2011<br>
> @@ -261,7 +261,7 @@<br>
> const VarDecl *D,<br>
> llvm::GlobalVariable *Addr) {<br>
> StartFunction(GlobalDecl(), getContext().VoidTy, Fn, FunctionArgList(),<br>
> - SourceLocation());<br>
> + SourceLocation(), CC_Default);<br>
><br>
> // Use guarded initialization if the global variable is weak due to<br>
> // being a class template's static data member.<br>
> @@ -278,7 +278,7 @@<br>
> llvm::Constant **Decls,<br>
> unsigned NumDecls) {<br>
> StartFunction(GlobalDecl(), getContext().VoidTy, Fn, FunctionArgList(),<br>
> - SourceLocation());<br>
> + SourceLocation(), CC_Default);<br>
><br>
> for (unsigned i = 0; i != NumDecls; ++i)<br>
> if (Decls[i])<br>
> @@ -290,8 +290,10 @@<br>
> void CodeGenFunction::GenerateCXXGlobalDtorFunc(llvm::Function *Fn,<br>
> const std::vector<std::pair<llvm::WeakVH, llvm::Constant*> ><br>
> &DtorsAndObjects) {<br>
> + const bool IsWin64 = getContext().Target.isWin64();<br>
> +<br>
> StartFunction(GlobalDecl(), getContext().VoidTy, Fn, FunctionArgList(),<br>
> - SourceLocation());<br>
> + SourceLocation(), IsWin64 ? CC_Win64ThisCall : CC_Default);<br>
<br>
</div></div>Again :)<br>
<br>
If we're going to keep this operation of "adjust actual calling convention for a member function" within CodeGen (as opposed to, say, moving it into Sema somehow), please abstract it into a function or make it part of the ABI abstraction layer.<br>
<br>
> [snipped a bunch more cases]<br>
<div class="im"><br>
<br>
> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=126863&r1=126862&r2=126863&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=126863&r1=126862&r2=126863&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)<br>
> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Wed Mar 2 13:36:23 2011<br>
> @@ -942,6 +942,26 @@<br>
><br>
> return false;<br>
> }<br>
> +<br>
> + void SetTargetAttributes(const Decl *D,<br>
> + llvm::GlobalValue *GV,<br>
> + CodeGen::CodeGenModule &M) const {<br>
> + if (M.getContext().Target.isWin64()) {<br>
> + if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {<br>
> + const CXXMethodDecl *M = dyn_cast<CXXMethodDecl>(FD);<br>
> +<br>
> + if ((M && M->isInstance())<br>
> + || dyn_cast<CXXConstructorDecl>(FD)<br>
> + || dyn_cast<CXXDestructorDecl>(FD)) {<br>
> + // When using the MSVC Win64 ABI, methods/constructors/destructors<br>
> + // use the Win64 thiscall calling convention.<br>
> + llvm::Function *F = cast<llvm::Function>(GV);<br>
> + F->setCallingConv(llvm::CallingConv::Win64_ThisCall);<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> };<br>
<br>
</div>This needs to check whether the calling convention has been overridden.<br>
<br>
I feel like there's a much cleaner solution than what is in this patch. The number of places where it's checking for Win64-specific behavior is unnerving, so at the very least we need to abstract that out into some more general function (e.g., getDefaultCallingConventionFor(Decl*)). Given that, and the buildbot breakage, it might be best to revert this commit and see how cleanly we can introduce this behavior.<br>
<br>
- Doug</blockquote></div><br>