This revision was automatically updated to reflect the committed changes.
Closed by commit rL285849: regcall: Implement regcall Calling Conv in clang
(authored by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D25204?vs=76717=76755#toc
Repository:
rL LLVM
rnk accepted this revision.
rnk added a reviewer: rnk.
rnk added a comment.
This revision is now accepted and ready to land.
Looks good to me.
https://reviews.llvm.org/D25204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
erichkeane updated this revision to Diff 76717.
erichkeane added a comment.
It was brought to my attention that regcall isn't a calling convention that
should cause MSVC to switch to C++ mangling. Switched that and updated the
tests.
https://reviews.llvm.org/D25204
Files:
erichkeane added a comment.
@rnk, @majnemer and @ABataev : I believe that I've done everything that has
come up in review, and this passes all tests for the convention I can find. Do
you guys see anything that is holding this patch up? What is otherwise the
'next step' in getting this into
erichkeane updated this revision to Diff 76117.
erichkeane added a comment.
Remove single-underscore version of _regcall kw.
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
rnk added a comment.
In https://reviews.llvm.org/D25204#581477, @erichkeane wrote:
> In general, I can see the benefit of this rule, however in the case of
> calling conventions, I would think that keeping them all orthogonal is
> important. Having "most" calling conventions work one way, and
erichkeane added a comment.
In https://reviews.llvm.org/D25204#581469, @rnk wrote:
> Remember the fight over _Atomic with MSVC's STL? The fallacy of the
> implementer's namespace is that there is only one implementer.
> https://llvm.org/bugs/show_bug.cgi?id=19043
>
> We should prefer adding
rnk added a comment.
Remember the fight over _Atomic with MSVC's STL? The fallacy of the
implementer's namespace is that there is only one implementer.
https://llvm.org/bugs/show_bug.cgi?id=19043
We should prefer adding `__attribute__`s and `__declspec`s instead of keywords
when possible.
joerg added a comment.
The __ namespace is shared between all parts of the implementation, not just
the compiler. The convention in the past was that compiler keywords will end in
a __ as well, but calling conventions and Objective C broke that convention. I
still think it is something that
majnemer added a comment.
The __ namespace is reserved for us and I can't imagine how __regcall would
upset any existing code out there.
https://reviews.llvm.org/D25204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
erichkeane added a comment.
I guess I'm not sure how to respond to that... Calling conventions
traditionally use double underscore to prevent from stomping on user keywords.
Additionally, this is in a specification that has an existing implementation
available, so I'm not sure what could be
joerg added a comment.
Can we please avoid adding more (pseudo) keywords in the double-underscore name
space? Those tend to be used a lot by existing libc implementations and
existing attribute cases like __strong and __weak have created enough trouble.
https://reviews.llvm.org/D25204
erichkeane updated this revision to Diff 75919.
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/Specifiers.h
include/clang/Basic/TokenKinds.def
lib/AST/Expr.cpp
erichkeane marked 8 inline comments as done.
erichkeane added a comment.
New diff coming that fixes Reid & David's comments
https://reviews.llvm.org/D25204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
majnemer added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:433
Out << Prefix;
+
mangleName(D);
Please remove this stray newline.
Comment at: lib/CodeGen/TargetInfo.cpp:
+ if (classifyArgumentType(FD->getType(),
+
rnk added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:1263
+On x86 targets, this attribute changes the calling convention to
+__regcall convention. This convention aims to pass as many arguments
+as possible in registers. It also tries to utilize registers for
erichkeane added a comment.
After much debate, the architects have agreed to change the "Decoration"
section to the following.
The next patch does these, so I'm ready for continued review. Thanks for your
patience!
-Erich
__regcall Decoration
Names of functions that use __regcall are
erichkeane updated this revision to Diff 75903.
erichkeane added a comment.
Corrected Decoration settings to match the soon-to-be-updated spec.
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
erichkeane added a comment.
I'm going to need to pump-the-brakes on this for a little bit. The
name-decoration work I did here highlighted a number of issues with that
section of the spec. We're currently considering rev'ing the spec to properly
name mangle/decorate when C++ and Microsoft is
erichkeane added a comment.
Quick point i meant to post earlier Couldn't change ExtInfo size.
Comment at: include/clang/AST/Type.h:1381
/// regparm and the calling convention.
-unsigned ExtInfo : 9;
+unsigned ExtInfo : 10;
ABataev wrote:
>
erichkeane updated this revision to Diff 73716.
erichkeane marked 6 inline comments as done.
erichkeane added a comment.
Updated tests to properly show mangling for C++ types. Required some fixes.
Note that the decorating of names doesn't match ICC in non-named functions due
to a bug in ICC,
erichkeane marked 9 inline comments as done.
erichkeane added inline comments.
> oren_ben_simhon wrote in AttrDocs.td:1267
> You might want to use the following link instead because it is most updated:
> https://software.intel.com/en-us/node/693069
This has changed 2x since I started this
ABataev added inline comments.
> Type.h:1381
> /// regparm and the calling convention.
> -unsigned ExtInfo : 9;
> +unsigned ExtInfo : 10;
>
Erich, do you really need this? You don't increase number of required bits
anymore, so this code must be restored
> Type.h:2909-2921
> +
oren_ben_simhon added inline comments.
> AttrDocs.td:1267
> +
> +.. _`__regcall`: https://software.intel.com/en-us/node/512847
> + }];
You might want to use the following link instead because it is most updated:
https://software.intel.com/en-us/node/693069
> TargetInfo.cpp:3352
>// Keep
erichkeane marked 16 inline comments as done.
erichkeane added a comment.
Commenting to save my comments (don't seem to survive a refresh). Still
working on non-function mangling.
> rnk wrote in ItaniumMangle.cpp:1203
> What mangling should happen for operator overloads and all other kinds
majnemer added inline comments.
> ItaniumMangle.cpp:1234
>
> - mangleSourceName(II);
> + auto FD = dyn_cast(ND);
> + bool isRegCall = (FD != nullptr) &&
`auto *`
> ItaniumMangle.cpp:1235
> + auto FD = dyn_cast(ND);
> + bool isRegCall = (FD != nullptr) &&
> +
majnemer added inline comments.
> ItaniumMangle.cpp:1413-1414
>
> -void CXXNameMangler::mangleSourceName(const IdentifierInfo *II) {
> - // ::=
> +void CXXNameMangler::mangleSourceName(const IdentifierInfo *II,
> + bool isRegCall) {
> + // ::=
rnk added inline comments.
> erichkeane wrote in ItaniumMangle.cpp:1236-1237
> Right, good catch. I looked at Mangle.cpp which does something very similar,
> and assumes that FunctionType is a valid cast here, so I've switched this
> here too, please let me know if that is a wrong assumption.
erichkeane added inline comments.
> rnk wrote in TargetInfo.cpp:3742-3743
> But, if the return value is returned directly, it doesn't conflict with the
> free parameter registers. In my example, the return value can use XMM0-3 and
> the parameters can use XMM0-15. Can you add this test case
rnk added inline comments.
> erichkeane wrote in TargetInfo.cpp:3742-3743
> That was my intent, this should allow return values to be in registers as
> well if I'm reading the spec correctly. The idea is that register use is
> 'greedy'.
But, if the return value is returned directly, it
erichkeane updated this revision to Diff 73507.
erichkeane marked an inline comment as done.
erichkeane added a comment.
Herald added a subscriber: dschuff.
Fixes based on Alexey/Ried's feedback
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
erichkeane marked 11 inline comments as done.
erichkeane added a comment.
New patch incoming.
> ABataev wrote in ItaniumMangle.cpp:1236-1237
> What if function type is not a FunctionProtoType?
Right, good catch. I looked at Mangle.cpp which does something very similar,
and assumes that
rnk added inline comments.
> AttrDocs.td:1263
> +On x86 targets, this attribute changes the calling convention to
> +__regcall convention. This convention aimes to pass as many arguments
> +as possible in registers. It also tries to utilize registers for the
"aims"
> TargetInfo.cpp:3306
> +
ABataev added inline comments.
> Index.h:3029
>CXCallingConv_PreserveAll = 15,
> + CXCallingConv_X86RegCall = 16,
>
Maybe it is better to use 8, as the previous comment allows it?
/* Value 8 was PnaclCall, but it was never used, so it could safely be
re-used. */
In this case you
erichkeane updated this revision to Diff 73490.
erichkeane added a comment.
Did a full context diff, as requested.
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
ABataev added a comment.
In https://reviews.llvm.org/D25204#560894, @erichkeane wrote:
> Hi Alexey-
> Can you let me know what you mean by "Full Context Review"? I'm unfamiliar
> with that process. The other fixes I'll look at today.
Check this page http://llvm.org/docs/Phabricator.html
erichkeane marked 4 inline comments as done.
erichkeane added a comment.
Updated the code, fixed Alexey's concerns. Thanks again for the comments!
https://reviews.llvm.org/D25204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
erichkeane removed rL LLVM as the repository for this revision.
erichkeane updated this revision to Diff 73489.
https://reviews.llvm.org/D25204
Files:
include/clang-c/Index.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
erichkeane added a comment.
Hi Alexey-
Can you let me know what you mean by "Full Context Review"? I'm unfamiliar
with that process. The other fixes I'll look at today.
Repository:
rL LLVM
https://reviews.llvm.org/D25204
___
cfe-commits
ABataev added a comment.
Eric, please, prepare a full context review!
> ItaniumMangle.cpp:1417-1421
> + if (isRegCall) {
> +Out << II->getLength() + sizeof("__regcall3__") - 1<< "__regcall3__";
> + } else {
> +Out << II->getLength();
> + }
Single-line substatements must not be
erichkeane created this revision.
erichkeane added reviewers: oren_ben_simhon, cfe-commits.
erichkeane set the repository for this revision to rL LLVM.
The Register Calling Convention (RegCall) was introduced by Intel to optimize
parameter transfer on function call.
This calling convention
41 matches
Mail list logo
