[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. Sorry, it's just been a busy month for me. In D89490#2539950 , @aguinet wrote: > In D89490#2516255 , @rjmccall wrote: > >> In D89490#2514695 ,

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. @rjmccall @mstorsjo @aaron.ballman any advice on what to do next? Should I bring this discussion back to llvm-dev? I don't want this to justs stall here. I would like to have a clear decision on why it is or it is not a good idea to merge this in LLVM. Thanks!

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. So, what could be the futur of that patch? Would that be okay to merge with a `target_abi` Clang attribute? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. In D89490#2516255 , @rjmccall wrote: > In D89490#2514695 , @aguinet wrote: > >>> I may be over-reacting to the way the patch seemed to be touching on the >>> C++ ABI in multiple places.

[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. In D89490#2514695 , @aguinet wrote: >> I may be over-reacting to the way the patch seemed to be touching on the C++ >> ABI in multiple places. My understanding is that `ms_abi` is just a >> calling-convention attribute; it's

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. > I may be over-reacting to the way the patch seemed to be touching on the C++ > ABI in multiple places. My understanding is that `ms_abi` is just a > calling-convention attribute; it's basically "use the (default) calling > convention that MSVC would use for this

[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. In D89490#2483792 , @aguinet wrote: > In D89490#2482531 , @rjmccall wrote: > >> I'm not calling Wine a niche use-case, I'm calling this feature a niche >> use-case. The lack of this

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. In D89490#2482531 , @rjmccall wrote: > I'm not calling Wine a niche use-case, I'm calling this feature a niche > use-case. The lack of this feature has not blocked Wine from being a > successful project. The feature has to

[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. In D89490#2481306 , @rjmccall wrote: > I do feel like this is a terrible idea, sorry. It's a *very* niche use case > to be dedicating a new compiler feature to, and it's a feature that demands a > lot from the internal compiler

[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't. If you really need to build code

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aaron.ballman added a comment. The frontend parts LGTM, but you should wait for someone else to review the backend parts before landing. Comment at: clang/test/Sema/callingconv-darwin_abi.c:10 + +void(__attribute__((darwin_abi)) * pfoo2)(void) = foo; //

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added inline comments. Comment at: clang/test/Sema/callingconv-darwin_abi.c:10 + +void(__attribute__((darwin_abi)) * pfoo2)(void) = foo; // expected-warning{{incompatible function pointer types}} aaron.ballman wrote: > You should also add some tests

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 314565. aguinet marked 4 inline comments as done. aguinet added a comment. Rebase on current master, and take into account @aaron.ballman 's fixes (thanks!). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. In D89490#2477937 , @emaste wrote: >> For now, only Linux/ARM64 is supported/tested. > > Is there any reason this is Linux-specific (as far as support; I understand > if it's not easy for you to test on non-Linux arm64). It's

[PATCH] D89490: Introduce __attribute__((darwin_abi))

emaste added a comment. > For now, only Linux/ARM64 is supported/tested. Is there any reason this is Linux-specific (as far as support; I understand if it's not easy for you to test on non-Linux arm64). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aaron.ballman added a comment. Some minor nits while double-checking the attribute bits. Comment at: clang/include/clang/Basic/AttrDocs.td:2334 + +Here is the list of whatthis attribute supports, quoting the aformentioned +document: Comment

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 313842. aguinet added a comment. Replace a just introduced `const auto` usage. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h clang/include/clang/Basic/Attr.td

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 313841. aguinet added a comment. Clang format fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 313838. aguinet added a comment. Rebased on current master branch, and added clang sema tests cc @aaron.ballman . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. Thanks @aaron.ballman for the comments! I fixed all your comments in the new patch. I will upload a new one with the Sema tests! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 310006. aguinet marked 3 inline comments as done. aguinet added a comment. Multiple things: - remove clang-format tags - remove const usage - enhanced attribute documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aaron.ballman added a reviewer: rjmccall. aaron.ballman added a subscriber: rjmccall. aaron.ballman added a comment. You should add some frontend Sema tests for the attribute. The usual tests are: correct usage without diagnostics (as both a declaration attribute and a type attribute), applying

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 308203. aguinet added a comment. Relax checks in `CodeGenCXX/darwinabi-returnthis.cpp` Clang test (to adapt to new attributes), and removes some useless brackets in if statements. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1481 +def DarwinABI : DeclOrTypeAttr { + let Spellings = [GCC<"darwin_abi">]; +// let Subjects = [Function, ObjCMethod]; aaron.ballman wrote: > I suspect this should be using the

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 308202. aguinet marked 10 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aaron.ballman added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:238 + if (D->hasAttr()) +return IsDarwin ? CC_C : CC_AArch64Darwin; + mstorsjo wrote: > aaron.ballman wrote: > > Can you help me understand this change a bit better? If the

[PATCH] D89490: Introduce __attribute__((darwin_abi))

mstorsjo added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:238 + if (D->hasAttr()) +return IsDarwin ? CC_C : CC_AArch64Darwin; + aaron.ballman wrote: > Can you help me understand this change a bit better? If the declaration uses > the Darwin

[PATCH] D89490: Introduce __attribute__((darwin_abi))

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

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 298569. aguinet added a comment. One missing formatting case... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 298568. aguinet added a comment. Add some clang-format tags, and restore back the "original" formatting for these cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490

[PATCH] D89490: Introduce __attribute__((darwin_abi))

mstorsjo added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:876 SDValue LowerAAPCS_VASTART(SDValue Op, SelectionDAG ) const; + SDValue LowerAAPCSFromDarwin_VASTART(SDValue Op, SelectionDAG ) const; SDValue LowerDarwin_VASTART(SDValue Op,

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added inline comments. Comment at: llvm/lib/AsmParser/LLParser.cpp:2147 +CC = CallingConv::Tail; +break; case lltok::kw_cc: { Again here this "big" diff is a result of clang-format. We can see that "kw_aarch64_sve_vector_pcs" has been

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 298548. aguinet added a comment. Fix one clang-tidy warning Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h

[PATCH] D89490: Introduce __attribute__((darwin_abi))

mstorsjo added inline comments. Comment at: llvm/lib/IR/AsmWriter.cpp:379 +Out << "aarch64_darwincc"; +break; case CallingConv::SPIR_FUNC: Out << "spir_func"; break; aguinet wrote: > mstorsjo wrote: > > The new code here has a different

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added inline comments. Comment at: llvm/lib/IR/AsmWriter.cpp:379 +Out << "aarch64_darwincc"; +break; case CallingConv::SPIR_FUNC: Out << "spir_func"; break; mstorsjo wrote: > The new code here has a different indentation than the rest; the

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet updated this revision to Diff 298461. aguinet added a comment. Update format + llvm tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89490/new/ https://reviews.llvm.org/D89490 Files: clang/include/clang-c/Index.h

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet added a comment. In D89490#2333073 , @mstorsjo wrote: > I see that the llvm side of the patch lacks tests? Sorry I forgot to add them indeed... I will push a patch with these. About the formatting, git clang-format HEAD~1 did this, and it looks

[PATCH] D89490: Introduce __attribute__((darwin_abi))

mstorsjo added a comment. I see that the llvm side of the patch lacks tests? Comment at: llvm/lib/IR/AsmWriter.cpp:379 +Out << "aarch64_darwincc"; +break; case CallingConv::SPIR_FUNC: Out << "spir_func"; break; The new code here has a different

[PATCH] D89490: Introduce __attribute__((darwin_abi))

aguinet created this revision. aguinet added reviewers: t.p.northover, mstorsjo, thegameg. Herald added subscribers: llvm-commits, cfe-commits, arphaman, dexonsmith, steven_wu, hiraditya, kristof.beyls, krytarowski. Herald added a reviewer: aaron.ballman. Herald added projects: clang, LLVM.