[PATCH] D41039: Add support for attribute "trivial_abi"

rsmith added a comment. In https://reviews.llvm.org/D41039#984009, @ahatanak wrote: > Yes, please document this in itanium-cxx-abi. Thanks! Looks like this hasn't happened yet. Repository: rL LLVM https://reviews.llvm.org/D41039 ___ cfe-commit

[PATCH] D41039: Add support for attribute "trivial_abi"

This revision was automatically updated to reflect the committed changes. Closed by commit rL324269: Add support for attribute 'trivial_abi'. (authored by ahatanak, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D41039?vs=129397&id=132

[PATCH] D41039: Add support for attribute "trivial_abi"

This revision was automatically updated to reflect the committed changes. ahatanak marked 2 inline comments as done. Closed by commit rC324269: Add support for attribute 'trivial_abi'. (authored by ahatanak, committed by ). Changed prior to commit: https://reviews.llvm.org/D41039?vs=129397&id=1

[PATCH] D41039: Add support for attribute "trivial_abi"

rsmith accepted this revision. rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:2240-2241 + enum TrivialityKind { +TK_NoTrivialABI, // The triviality of a method unaffected by "trivial_abi". +TK_TrivialABI // The triviality of a method affected by "tri

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak added a comment. Yes, please document this in itanium-cxx-abi. Thanks! https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall accepted this revision. rjmccall added a comment. Looks great to me! Thanks for taking this on, it's a pretty major improvement for users. Would you like to create an issue with itanium-cxx-abi to document this, or do you want me to handle that? https://reviews.llvm.org/D41039 __

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak added a comment. I think I've addressed all the review comments. Any other comments from anyone? https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 129397. ahatanak added a comment. In CXXRecordDecl::addedMember, set HasTrivialSpecialMembersForCall if Method->isTrivialForCall() returns true. This fixes a bug where CXXRecordDecl::hasNonTrivialDestructorForCall would return false for the implicit destr

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11700 +} + } + ahatanak wrote: > rsmith wrote: > > rjmccall wrote: > > > I think it's correct not to call CheckDestructorAccess and > > > DiagnoseUseOfDecl here, since according to the standa

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 129345. ahatanak marked 2 inline comments as done. ahatanak added a comment. Partially revert the changes I made to CodeGenFunction::EmitParmDecl add IRGen test case for exception handling. https://reviews.llvm.org/D41039 Files: include/clang/AST/ASTCon

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked 7 inline comments as done. ahatanak added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11700 +} + } + rsmith wrote: > rjmccall wrote: > > I think it's correct not to call CheckDestructorAccess and > > DiagnoseUseOfDecl here, since acco

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 129038. ahatanak added a comment. Add more tests for template instantiation of "trivial_abi" classes. https://reviews.llvm.org/D41039 Files: include/clang/AST/ASTContext.h include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/AST/Type.

[PATCH] D41039: Add support for attribute "trivial_abi"

rsmith added a comment. I'd like to see more testing for the template instantiation case. I don't see any test coverage for the "attribute only affects instantiations whose members are trivial-for-calls" part. Comment at: include/clang/Sema/Sema.h:2239 bool SpecialMemberIs

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks good to me. https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128960. ahatanak marked 7 inline comments as done. ahatanak added a comment. Address review comments. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/AST/Type.h include/clang/Basic/Attr.td

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:811 + bool hasTrivialABIOverride() const; + This should get a comment, even if it's just to refer to the CXXRecordDecl method. Comment at: lib/CodeGen/CGCall.cpp:3498 b

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked 4 inline comments as done. ahatanak added a comment. In https://reviews.llvm.org/D41039#969171, @rjmccall wrote: > I'll trust Richard on the tricky Sema/AST bits. The functionality of the > patch looks basically acceptable to me, although I'm still not thrilled about > the idea

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128895. ahatanak added a comment. Address review comments. Also, emit the declaration of the destructor of a trivial-abi override class in Sema::ActOnParamDeclarator and mark it as referenced. This is necessary because a trivial-abi type that is passed by

[PATCH] D41039: Add support for attribute "trivial_abi"

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1159 +def TrivialABI : InheritableAttr { + let Spellings = [Clang<"trivial_abi">]; + let Subjects = SubjectList<[CXXRecord]>; Would this attribute make sense in C, or does it really on

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added a comment. I'll trust Richard on the tricky Sema/AST bits. The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect. But we could c

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128839. ahatanak marked 3 inline comments as done. ahatanak added a comment. Rename variable to HasTrivialABIOverride. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/Basic/Attr.td include/c

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked an inline comment as done. ahatanak added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:833 // passed in registers, which is non-conforming. if (RD->hasNonTrivialDestructor() && getContext().getTypeSize(RD->getTypeForDecl()) > 64)

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128838. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/AST/DeclCXX

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128815. ahatanak marked an inline comment as done. ahatanak added a comment. Check whether the argument is passed indirectly before pushing the cleanup. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h inclu

[PATCH] D41039: Add support for attribute "trivial_abi"

rsmith added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3498 bool HasAggregateEvalKind = hasAggregateEvaluationKind(type); + bool TypeDestructedInCallee = false; + if (const auto *RD = type->getAsCXXRecordDecl()) This is a confusing name for this fl

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked 2 inline comments as done. ahatanak added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1827 + + if (!IsScalar && !CurFuncIsThunk && + (getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee() || This is not correct since it isn't

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked 3 inline comments as done. ahatanak added inline comments. Comment at: include/clang/AST/DeclCXX.h:1489-1491 + bool shouldBeDestructedInCallee() const { +return data().CanPassInRegisters && hasNonTrivialDestructor(); + } rsmith wrote: > This

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128798. ahatanak marked an inline comment as done. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D41039: Add support for attribute "trivial_abi"

rsmith added inline comments. Comment at: include/clang/AST/DeclCXX.h:1489-1491 + bool shouldBeDestructedInCallee() const { +return data().CanPassInRegisters && hasNonTrivialDestructor(); + } This will return incorrect results on MSVC, where every class typ

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128784. ahatanak added a comment. - Serialize/deserialize the bits I added to FunctionDecl and CXXRecordDecl. - Enable passing non-trivial structs when clang abi-compat version is 4.0 or lower. - Fix a bug in Sema::CheckCompletedCXXClass where the CXXRecordD

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak added a comment. Sorry, the patch I've just uploaded was missing some changes I made. https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128781. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/AST/Type.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Se

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked an inline comment as done. ahatanak added inline comments. Comment at: include/clang/Basic/AttrDocs.td:2261 +- The class or its subobjects have Objective-C pointer type members and ARC is + enabled. + }]; rjmccall wrote: > I think the right list

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak marked 10 inline comments as done. ahatanak added inline comments. Comment at: include/clang/AST/DeclCXX.h:443 +/// SMF_MoveConstructor, and SMF_Destructor are meaningful here. +unsigned HasTrivialSpecialMembersForCall : 6; + rjmccall wrote: > I

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128767. ahatanak added a comment. Address review comments. I also made changes so that FunctionDecl::IsTrivialForCall is always set to true for special functions of "trivial_abi" classes. There is still one microsoft IRGen test failing because I haven't im

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added a comment. In https://reviews.llvm.org/D41039#951807, @rjmccall wrote: > In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: > > > I had a discussion with Duncan today and he pointed out that perhaps we > > shouldn't allow users to annotate a struct with "trivial_abi" if o

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128694. ahatanak added a comment. No worries. Upload the patch again. https://reviews.llvm.org/D41039 Files: include/clang/AST/Decl.h include/clang/AST/DeclCXX.h include/clang/AST/Type.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td

[PATCH] D41039: Add support for attribute "trivial_abi"

aprantl added a comment. Unfortunately it looks like I do not have permission to re-upload Akira's last patch to this review. Repository: rL LLVM https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://list

[PATCH] D41039: Add support for attribute "trivial_abi"

aprantl reopened this revision. aprantl added a comment. I'm sorry, I made a copy&paste error in a the commit message for https://reviews.llvm.org/D41743 and accidentally associated that commit with this review. I suppose I should learn to use arcanist. Reopening, and my apologies for the noise!

[PATCH] D41039: Add support for attribute "trivial_abi"

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL321845: Debug Info: Support DW_AT_calling_convention on composite types. (authored by adrian, committed by ). Changed pri

[PATCH] D41039: Add support for attribute "trivial_abi"

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rC321845: Debug Info: Support DW_AT_calling_convention on composite types. (authored by adrian, committed by ). Changed pri

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak updated this revision to Diff 128642. ahatanak added a comment. I've only fixed the places where the bits to track the triviality of special functions are set or reset, so this is still a WIP. I'll update the patch again later today, but let me know if anyone has any feedback in the mea

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

> On Jan 3, 2018, at 6:39 PM, John McCall wrote: > > On Wed, Jan 3, 2018 at 2:07 PM, Akira Hatanaka > wrote: >> On Jan 3, 2018, at 10:25 AM, John McCall > > wrote: >> >> On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka >

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Wed, Jan 3, 2018 at 2:07 PM, Akira Hatanaka wrote: > On Jan 3, 2018, at 10:25 AM, John McCall wrote: > > On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka > wrote: > >> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >> >> >> On Tue, Dec 19,

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

> On Jan 3, 2018, at 10:25 AM, John McCall wrote: > > On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka > wrote: >> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits >> mailto:cfe-commits@lists.llvm.org>> wrote: >> >> >> >> On Tue, Dec 19, 2017 at 9:43 PM

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka wrote: > On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > > > On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka wrote: > >> On Tue, Dec 12, 2017 at 12:12 PM, John McCall wrote: >> >>> On Tue, Dec 12

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits > wrote: > > > > On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka > wrote: > On Tue, Dec 12, 2017 at 12:12 PM, John McCall > wrote: > On Tue, Dec 12, 2017 at 1:45 PM, David Blaiki

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka wrote: > On Tue, Dec 12, 2017 at 12:12 PM, John McCall wrote: > >> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie >> wrote: >> >>> On Mon, Dec 11, 2017 at 5:38 PM John McCall wrote: >>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie wro

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Tue, Dec 12, 2017 at 12:12 PM, John McCall wrote: > On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie wrote: > >> On Mon, Dec 11, 2017 at 5:38 PM John McCall wrote: >> >>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie >>> wrote: >>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabric

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie wrote: > On Mon, Dec 11, 2017 at 5:38 PM John McCall wrote: > >> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie >> wrote: >> >>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < >>> revi...@reviews.llvm.org> wrote: >>> rjmccall ad

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Mon, Dec 11, 2017 at 5:38 PM John McCall wrote: > On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie wrote: > >> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >>> rjmccall added a comment. >>> >>> In https://reviews.llvm.org/D41039#951648, @a

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie wrote: > On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < > revi...@reviews.llvm.org> wrote: > >> rjmccall added a comment. >> >> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: >> >> > I had a discussion with Duncan today an

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < revi...@reviews.llvm.org> wrote: > rjmccall added a comment. > > In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: > > > I had a discussion with Duncan today and he pointed out that perhaps we > shouldn't allow users to annota

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added a comment. In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: > I had a discussion with Duncan today and he pointed out that perhaps we > shouldn't allow users to annotate a struct with "trivial_abi" if one of its > subobjects is non-trivial and is not annotated with "tr

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

My bet would be: warn and ignore it, but probably Richard's & John might have stronger thoughts/justifications/etc. On Mon, Dec 11, 2017 at 1:38 PM Akira Hatanaka via Phabricator < revi...@reviews.llvm.org> wrote: > ahatanak added a comment. > > I had a discussion with Duncan today and he pointed

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak added a comment. I had a discussion with Duncan today and he pointed out that perhaps we shouldn't allow users to annotate a struct with "trivial_abi" if one of its subobjects is non-trivial and is not annotated with "trivial_abi" since that gives users too much power. Should we error

[PATCH] D41039: Add support for attribute "trivial_abi"

ahatanak created this revision. Herald added a subscriber: javed.absar. This patch adds support for a new attribute "trivial_abi", which will be used to instruct clang to pass and return non-trivial C++ structs directly when it's possible to do so. The original RFC I sent to cfe-dev last month i