[NFC] Remove getDefaultCallingConvention IsBuiltin #145904
Conversation
ASTContext::getDefaultCallingConvention() was documented as returning "the default calling convention for the current target", but did not do this, and was never intended to do this, it has always been controlled by command-line options to deviate from the target default. This commit changes ASTContext::getDefaultCallingConvention() to reflect the fact that it returns the context's default calling convention, not the target's default calling convention. The IsBuiltin parameter, which was used to return the target's default calling convention rather than the context's, is removed in favor of getTargetInfo().getDefaultCallingConv() which is more explicit of the intent.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-risc-v Author: Harald van Dijk (hvdijk) ChangesASTContext::getDefaultCallingConvention() was documented as returning "the default calling convention for the current target", but did not do this, and was never intended to do this, it has always been controlled by command-line options to deviate from the target default. This commit changes ASTContext::getDefaultCallingConvention() to reflect the fact that it returns the context's default calling convention, not the target's default calling convention. The IsBuiltin parameter, which was used to return the target's default calling convention rather than the context's, is removed in favor of Full diff: https://github.com/llvm/llvm-project/pull/145904.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 2b9cd035623cc..d374ec6e72a71 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2914,10 +2914,14 @@ class ASTContext : public RefCountedBase<ASTContext> {
NestedNameSpecifier *
getCanonicalNestedNameSpecifier(NestedNameSpecifier *NNS) const;
- /// Retrieves the default calling convention for the current target.
+ /// Retrieves the default calling convention for the current context.
+ ///
+ /// The context's default calling convention may differ from the current
+ /// target's default calling convention if the -fdefault-calling-conv option
+ /// is used; to get the target's default calling convention, e.g. for built-in
+ /// functions, call getTargetInfo().getDefaultCallingConv() instead.
CallingConv getDefaultCallingConvention(bool IsVariadic,
- bool IsCXXMethod,
- bool IsBuiltin = false) const;
+ bool IsCXXMethod) const;
/// Retrieves the "canonical" template name that refers to a
/// given template.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index e7abb18330e17..aa55018ddd249 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -12677,10 +12677,9 @@ QualType ASTContext::GetBuiltinType(unsigned Id,
bool Variadic = (TypeStr[0] == '.');
- FunctionType::ExtInfo EI(getDefaultCallingConvention(
- Variadic, /*IsCXXMethod=*/false, /*IsBuiltin=*/true));
- if (BuiltinInfo.isNoReturn(Id)) EI = EI.withNoReturn(true);
-
+ FunctionType::ExtInfo EI(Target->getDefaultCallingConv());
+ if (BuiltinInfo.isNoReturn(Id))
+ EI = EI.withNoReturn(true);
// We really shouldn't be making a no-proto type here.
if (ArgTypes.empty() && Variadic && !getLangOpts().requiresStrictPrototypes())
@@ -13064,43 +13063,38 @@ void ASTContext::forEachMultiversionedFunctionVersion(
}
CallingConv ASTContext::getDefaultCallingConvention(bool IsVariadic,
- bool IsCXXMethod,
- bool IsBuiltin) const {
+ bool IsCXXMethod) const {
// Pass through to the C++ ABI object
if (IsCXXMethod)
return ABI->getDefaultMethodCallConv(IsVariadic);
- // Builtins ignore user-specified default calling convention and remain the
- // Target's default calling convention.
- if (!IsBuiltin) {
- switch (LangOpts.getDefaultCallingConv()) {
- case LangOptions::DCC_None:
- break;
- case LangOptions::DCC_CDecl:
- return CC_C;
- case LangOptions::DCC_FastCall:
- if (getTargetInfo().hasFeature("sse2") && !IsVariadic)
- return CC_X86FastCall;
- break;
- case LangOptions::DCC_StdCall:
- if (!IsVariadic)
- return CC_X86StdCall;
- break;
- case LangOptions::DCC_VectorCall:
- // __vectorcall cannot be applied to variadic functions.
- if (!IsVariadic)
- return CC_X86VectorCall;
- break;
- case LangOptions::DCC_RegCall:
- // __regcall cannot be applied to variadic functions.
- if (!IsVariadic)
- return CC_X86RegCall;
- break;
- case LangOptions::DCC_RtdCall:
- if (!IsVariadic)
- return CC_M68kRTD;
- break;
- }
+ switch (LangOpts.getDefaultCallingConv()) {
+ case LangOptions::DCC_None:
+ break;
+ case LangOptions::DCC_CDecl:
+ return CC_C;
+ case LangOptions::DCC_FastCall:
+ if (getTargetInfo().hasFeature("sse2") && !IsVariadic)
+ return CC_X86FastCall;
+ break;
+ case LangOptions::DCC_StdCall:
+ if (!IsVariadic)
+ return CC_X86StdCall;
+ break;
+ case LangOptions::DCC_VectorCall:
+ // __vectorcall cannot be applied to variadic functions.
+ if (!IsVariadic)
+ return CC_X86VectorCall;
+ break;
+ case LangOptions::DCC_RegCall:
+ // __regcall cannot be applied to variadic functions.
+ if (!IsVariadic)
+ return CC_X86RegCall;
+ break;
+ case LangOptions::DCC_RtdCall:
+ if (!IsVariadic)
+ return CC_M68kRTD;
+ break;
}
return Target->getDefaultCallingConv();
}
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 4a86cbd0633b6..cfdf855f65ff1 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3467,8 +3467,8 @@ void Sema::DeclareGlobalAllocationFunction(DeclarationName Name,
}
}
- FunctionProtoType::ExtProtoInfo EPI(Context.getDefaultCallingConvention(
- /*IsVariadic=*/false, /*IsCXXMethod=*/false, /*IsBuiltin=*/true));
+ FunctionProtoType::ExtProtoInfo EPI(
+ Context.getTargetInfo().getDefaultCallingConv());
QualType BadAllocType;
bool HasBadAllocExceptionSpec = Name.isAnyOperatorNew();
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index aa7191d2814fe..6d6e07a2c03c7 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -22,6 +22,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/TargetInfo.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/ModuleLoader.h"
#include "clang/Lex/Preprocessor.h"
@@ -777,7 +778,7 @@ static void GetOpenCLBuiltinFctOverloads(
std::vector<QualType> &FunctionList, SmallVector<QualType, 1> &RetTypes,
SmallVector<SmallVector<QualType, 1>, 5> &ArgTypes) {
FunctionProtoType::ExtProtoInfo PI(
- Context.getDefaultCallingConvention(false, false, true));
+ Context.getTargetInfo().getDefaultCallingConv());
PI.Variadic = false;
// Do not attempt to create any FunctionTypes if there are no return types,
diff --git a/clang/lib/Sema/SemaRISCV.cpp b/clang/lib/Sema/SemaRISCV.cpp
index 9eab0c2a0df6a..c8b925156ed0b 100644
--- a/clang/lib/Sema/SemaRISCV.cpp
+++ b/clang/lib/Sema/SemaRISCV.cpp
@@ -417,7 +417,7 @@ void RISCVIntrinsicManagerImpl::CreateRVVIntrinsicDecl(LookupResult &LR,
ArgTypes.push_back(RVVType2Qual(Context, Sigs[i]));
FunctionProtoType::ExtProtoInfo PI(
- Context.getDefaultCallingConvention(false, false, true));
+ Context.getTargetInfo().getDefaultCallingConv());
PI.Variadic = false;
|
|
Additional context: although I see this as a code cleanup for LLVM, my primary motivation for submitting this is to make it easier for downstream users to have some built-ins use other calling conventions. |
|
@erichkeane Since you commited 0002281 which was the last significant change to the function, would you be able to comment on whether this seems reasonable to you? |
There was a problem hiding this comment.
This seems reasonable to me, though my knowledge of this is limited (I might be in the blame becasue I did a huge refactor here in the past).
@efriedma-quic perhaps has a comment?
|
"IsBuiltin" doesn't really mean whether something is "builtin", in whatever sense. It means "bypass Windows calling convention flags", which happens to apply to some functions we consider builtin. I suspect we're actually applying the bit too widely; it should probably only affect some very narrow set of functions, not arbitrary libcalls. I think refactoring like this makes things less clear. |
|
The way "IsBuiltin" is currently used, it means "the declaration is provided by Clang internally rather than by (or in addition to) a source or header file": those are the only places where If you want to reduce which declarations |
|
rvv intrinsics don't have a real calling convention at all; they're internally lowered by the compiler, so the calling convention doesn't actually matter. I suspect the same is true for opencl builtins. Looking again, I guess for C library libcalls, we assume the user's C library actually marks everything __cdecl. Or the user is building with -ffreestanding. I guess that's self-consistent, so we actually do want to pass IsBuiltin. This leaves two places where IsBuiltin actually has an effect we want. And each of those probably needs a comment explaining what's happening, so we don't copy-paste the code elsewhere. I guess if we have that comment, it doesn't matter that much whether we call getDefaultCallingConvention with IsBuiltin=false, or bypass it to use getTargetInfo().getDefaultCallingConv(), but going through getDefaultCallingConvention() with the flag makes it more obvious we're doing something unusual. |
|
The calling convention doesn't just apply at the LLVM level though. For that, you're right it wouldn't make a difference for RVV intrinsics, but it also applies at the Clang level to determine whether Clang passes arguments by value or by reference, and whether it promotes arguments. Would it not still be important for that for RVV intrinsics? For OpenCL builtins, this definitely does makes a difference. The OpenCL builtins aren't like LLVM intrinsics, they're library functions provided by libclc or equivalent and the same argument as for libc functions applies: libclc would be expected to be compiled without calling convention-affecting options, and if a user's kernel is compiled with calling convention-affecting options, things again break if the caller and callee side don't agree on which ABI to use for built-ins. I do think the effect of IsBuiltin is exactly what we want in all places it's currently used.
If we want this for all built-in functions, getTargetInfo().getDefaultCallingConv() becomes the normal thing to call there, it wouldn't be unusual and wouldn't need to be called out specifically. If we only want this for some built-in functions, we have a bigger mismatch between documentation and implementation. If the parameter is called |
Pretty sure we bypass all the normal calling convention lowering, in this context.
Oh, okay. It's probably fine to use "builtin" mode for all builtins; the expected semantics just wasn't really clear at first glance. Really what I don't want happening is someone copy-pastes |
Ah, this is confusing. There's two types of RVV intrinsics that use this code. Many use So, I think
That makes sense. I amended but there is no declaration context for it to take into account. |
4 participants
ASTContext::getDefaultCallingConvention() was documented as returning "the default calling convention for the current target", but did not do this, and was never intended to do this, it has always been controlled by command-line options to deviate from the target default.
This commit changes ASTContext::getDefaultCallingConvention() to reflect the fact that it returns the context's default calling convention, not the target's default calling convention. The IsBuiltin parameter, which was used to return the target's default calling convention rather than the context's, is removed in favor of
getTargetInfo().getDefaultCallingConv() which is more explicit of the intent.