aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1481
+def DarwinABI : DeclOrTypeAttr {
+ let Spellings = [GCC<"darwin_abi">];
+// let Subjects = [Function, ObjCMethod];
----------------
I suspect this should be using the `Clang` spelling as I don't believe GCC
supports this attribute.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:2258
+ let Content = [{
+On Linux ARM64 targets, this attribute changes the calling convention of
+a function to match the default convention used on Apple ARM64. This
----------------
Adding more details about what that ABI actually is, or linking to a place
where the user can learn more on their own, would be appreciated.
================
Comment at: clang/include/clang/Basic/Specifiers.h:269
+ // clang-format off
/// CallingConv - Specifies the calling convention that a function uses.
----------------
My personal opinion is that these formatting markers are noise. However,
there's a fair number of enumerations this could cover and disabling the format
behavior may be reasonable.
I think I'd rather see the formatting comments removed from this patch. They
can be added in a different commit but perhaps the file can be restructured so
that we don't feel the need to sprinkle so many comments around.
================
Comment at: clang/lib/AST/Type.cpp:3115
+// clang-format off
StringRef FunctionType::getNameForCallConv(CallingConv CC) {
----------------
I'd remove these as well.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:46
+// clang-format off
unsigned CodeGenTypes::ClangCallConvToLLVMCallConv(CallingConv CC) {
----------------
These too.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:238
+ if (D->hasAttr<DarwinABIAttr>())
+ return IsDarwin ? CC_C : CC_AArch64Darwin;
+
----------------
Can you help me understand this change a bit better? If the declaration uses
the Darwin ABI and the platform is Darwin, you want to use the cdecl convention?
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:70
+ // Returns true if AArch64 Darwin ABI is explicitly used.
+ const bool IsCtorOrDtor = (isa<CXXConstructorDecl>(GD.getDecl()) ||
+ (isa<CXXDestructorDecl>(GD.getDecl()) &&
----------------
We don't typically go with top-level `const` on non-pointer/reference types, so
I'd drop the `const`.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:78-79
+ cast<FunctionDecl>(GD.getDecl())->getType()->getAs<FunctionType>();
+ const auto FCC = FTy->getCallConv();
+ return FCC == CallingConv::CC_AArch64Darwin;
+ }
----------------
`return FTy->getCallConv() == CallingConv::CC_Aarch64Darwin;`
(Removes a questionable use of `auto` and a top-level `const` at the same time.)
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5449
bool isDarwinPCS() const { return Kind == DarwinPCS; }
+ bool isDarwinPCS(const unsigned CConv) const {
+ return isDarwinPCS() || CConv == llvm::CallingConv::AArch64Darwin;
----------------
Drop the top-level `const` in the parameter.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4640
+ case ParsedAttr::AT_DarwinABI:
+ CC = Context.getTargetInfo().getTriple().isOSDarwin() ? CC_C
+ : CC_AArch64Darwin;
----------------
Similar confusion on my part here -- if the OS is Darwin, we want to default to
cdecl?
================
Comment at: llvm/lib/IR/AsmWriter.cpp:379
+ Out << "aarch64_darwincc";
+ break;
case CallingConv::SPIR_FUNC: Out << "spir_func"; break;
----------------
mstorsjo wrote:
> aguinet wrote:
> > mstorsjo wrote:
> > > The new code here has a different indentation than the rest; the same in
> > > a couple other places throughout the patch.
> > As clang-format changed that formatting for me, should I reformat the whole
> > switch-case to be "clang-format compatible", or should we set this section
> > as ignored?
> As the manually laid out custom formatting here is kinda beneficial, I think
> it's preferred to keep it as is. If there are annotations to make
> clang-format stay off of it, that's probably good (but I don't know what our
> policy regarding use of such annotations is, if we have any).
FWIW, I'm on the side of "don't use the comments to turn off clang-format"
because it adds an extra two lines of noise every time we have to use the
markup. I'd rather see either clang-format improved for the cases that cause us
pain or the code change to be fine with what's produced by clang-format for
most cases (there will always be one-offs where it makes sense to use the
comments, I'm sure).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89490/new/
https://reviews.llvm.org/D89490
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
