erichkeane created this revision.
GCC's attribute 'target', in addition to being an optimization hint,
also allows function multiversioning. We currently have the former
implemented, this is the latter's implementation.
Note that it ends up having to permit redefinition of functions so
that they
hfinkel added a comment.
Can you explain briefly what is required of the system in order to support this
(is it just ifunc support in the dynamic linker / loader)?
In general, there are a number of places in this patch, at the Sema layer
(including in the error messages), that talk about how th
erichkeane added a comment.
In https://reviews.llvm.org/D38596#889682, @hfinkel wrote:
> Can you explain briefly what is required of the system in order to support
> this (is it just ifunc support in the dynamic linker / loader)?
Yep, this feature depends entirely on ifuncs.
> In general, the
hfinkel added a comment.
In https://reviews.llvm.org/D38596#889697, @erichkeane wrote:
> In https://reviews.llvm.org/D38596#889682, @hfinkel wrote:
>
> > Can you explain briefly what is required of the system in order to support
> > this (is it just ifunc support in the dynamic linker / loader)?
echristo added a comment.
This generally works for me modulo the things that Hal has mentioned. You'll
probably want to add Richard to the review list for the Sema bits as well.
Thanks!
Comment at: lib/Sema/SemaDecl.cpp:3214
if (!isFriend && NewMethod->getLexicalDeclCo
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.
Comment at: include/clang/AST/Decl.h:2226
+ MultiVersionKind getMultiVersionKind() const {
+return static_cast(this->MultiVersion);
+ }
Drop the `this->`
erichkeane marked 31 inline comments as done.
erichkeane added a subscriber: rnk.
erichkeane added a comment.
Weew... I think I got everything. Thanks for the review you three! I've got
another patch coming momentarily with what I believe is all your suggestions.
Comment at:
erichkeane updated this revision to Diff 118058.
erichkeane marked 5 inline comments as done.
erichkeane added a comment.
All requested changes AFAIK. Also, discovered that I wasn't calling cpu-init
at the beginning of my resolver, so added that.
Note: Virtual functions seem to have a bunch of
aaron.ballman added a comment.
The attribute and sema bits look good to me, but I agree that you might want
Richard's opinions before committing.
Comment at: lib/Sema/SemaDecl.cpp:9264
+
+ if (auto *CMD = dyn_cast(FD))
+if (CMD->isVirtual()) {
`const auto
erichkeane added a subscriber: rsmith.
erichkeane added a comment.
In https://reviews.llvm.org/D38596#891603, @aaron.ballman wrote:
> The attribute and sema bits look good to me, but I agree that you might want
> Richard's opinions before committing.
Oh, definitely! I actually put this up for
erichkeane updated this revision to Diff 118218.
erichkeane added reviewers: rnk, rsmith.
erichkeane added a comment.
1 more const-auto. Also noticed I'd missed that removing the 'default' caused
the function to fallthrough, so I added llvm_unreachable to the bottom.
https://reviews.llvm.org/D
erichkeane updated this revision to Diff 118648.
erichkeane added a comment.
Craig noticed a pair of ordering issues in the TargetArray list, so fixed htose.
https://reviews.llvm.org/D38596
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds
erichkeane added a comment.
@rsmith and @rnk just a quick bump! I'd like to get your thoughts on the SEMA
changes here.
Thanks!
-Erich
https://reviews.llvm.org/D38596
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/c
erichkeane updated this revision to Diff 120278.
erichkeane added a comment.
Just a quick rebase, there were a few changes in the area.
https://reviews.llvm.org/D38596
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Ba
erichkeane added a comment.
I note my rebase lost the tests... I'll fix that up soon, sorry guys!
https://reviews.llvm.org/D38596
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
erichkeane updated this revision to Diff 120460.
https://reviews.llvm.org/D38596
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clang/Sema/Sema.h
lib/Basic/Targets/X86.cpp
lib/Basic/Targ
erichkeane updated this revision to Diff 120513.
erichkeane added a comment.
Realized I can put variables in an Attribute, so I put MV info in the Attribute
rather than at the FD level.
@AaronBallman and @echristo, if you could take another look, I'd appreciate it.
https://reviews.llvm.org/D38
aaron.ballman added inline comments.
Comment at: include/clang/Basic/Attr.td:1917
+ MVK_All, // All Decls of this function have a 'target' attribute. None
differ
+ // in contents, so this is the 'hint' case.
+ MVK_MultiVersion, // All Decls of this function
erichkeane marked 2 inline comments as done.
erichkeane added inline comments.
Comment at: include/clang/Basic/Attr.td:1917
+ MVK_All, // All Decls of this function have a 'target' attribute. None
differ
+ // in contents, so this is the 'hint' case.
+ MVK_Mu
erichkeane updated this revision to Diff 120636.
erichkeane added a comment.
Fix comment in Attr.td that @aaron.ballman brought up on IRC.
https://reviews.llvm.org/D38596
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang
aaron.ballman added a comment.
I think the attribute and sema parts look good, but I leave the codegen bits to
someone with more experience in that area.
https://reviews.llvm.org/D38596
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:/
erichkeane updated this revision to Diff 120620.
erichkeane added a comment.
Fixes for Aaron's comments.
https://reviews.llvm.org/D38596
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clan
rsmith added a comment.
I'm not entirely happy with the AST representation you're using here. Allowing
multiple declarations of the same entity to have (semantically distinct) bodies
breaks our AST invariants, and will cause things like our PCH / modules support
to fail. This can probably be ma
erichkeane added a comment.
Hi Richard, thanks for the review! I'll make another attempt with this based
on your feedback, though if you could clarify your suggestion, it would be
greatly appreciated.
In https://reviews.llvm.org/D38596#909957, @rsmith wrote:
> I'm not entirely happy with the
rsmith added a comment.
In https://reviews.llvm.org/D38596#910035, @erichkeane wrote:
> > You should add tests for:
> >
> > - the PCH / Modules case (particularly regarding what happens when merging
> > multiple copies of the same set of functions, for instance when they're
> > defined inline)
rsmith added inline comments.
Comment at: lib/Basic/Targets/X86.cpp:1329
+ // implementation in in GCC 7.x in gcc/config/i386/i386.c
+ // ::get_builtin_code_for_version. This list is simplified from that source.
+ const auto TargetArray = {"avx512vpopcntdq",
e
erichkeane added a comment.
In https://reviews.llvm.org/D38596#913322, @rsmith wrote:
> The big pain point here is that we don't know whether a function is
> multiversioned until we see the *second* version of it (or a declaration with
> no target attribute or a declaration with a "default" tar
erichkeane abandoned this revision.
erichkeane added a comment.
Completely superceededby this patch here: https://reviews.llvm.org/D40819
Since that one is a complete redesign/reimplementation, I'd prefer to do it
separately.
https://reviews.llvm.org/D38596
_
28 matches
Mail list logo
