erichkeane added a comment.
I'm concerned as to the design of this addition, I don't particularly
appreciate the reasons for making 'target_clones' different, nor the purpose
for adding a new attribute instead of using 'target' for what seems like
exactly that? IF the new spelling is THAT necessary, we perhaps don't need a
whole new attribute for it either.
================
Comment at: clang/include/clang/AST/ASTContext.h:3090
+ std::vector<std::string>
+ filterFunctionTargetVersionAttrs(const TargetVersionAttr *TV) const;
----------------
It is concerning that this differs from the above.
================
Comment at: clang/include/clang/AST/Decl.h:1860
+ TargetClones,
+ TargetVersion
};
----------------
I find myself immediately wondering why this isn't just 'Target'. What
semantically are we trying to change?
================
Comment at: clang/include/clang/Basic/Attr.td:2738
+ let AdditionalMembers = [{
+ StringRef getTVName() const { return getNamesStr().trim(); }
+ bool isDefaultVersion() const {
----------------
would probably just call this getName ?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:2351
+def TargetVersionDocs : Documentation {
+ let Category = DocCatFunction;
----------------
This sounds absurdly like 'target', and I think we should instead investigate
why enabling THAT for ARM isn't sufficient.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:2374
+For AArch64 target:
+The attribute contains comma-separated strings of target features joined by "+"
----------------
This seems like an unnecessary change, we should do what we can to make this
work as much like the non-ARM version as possible.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11483
+def warn_target_clone_no_impact_options
+ : Warning<"version list contains no code impact entries">,
+ InGroup<FunctionMultiVersioning>;
----------------
I'm not clear as to what this means?
================
Comment at: clang/include/clang/Sema/Sema.h:4471
+ bool
+ checkTargetClonesAttrString(SourceLocation LiteralLoc, StringRef Str,
+ const StringLiteral *Literal, bool &HasDefault,
----------------
This further concerns me that we're changing behavior this much...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127812/new/
https://reviews.llvm.org/D127812
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
