[PATCH] D127812: [AArch64] Function multiversioning support added.

[Erich Keane via Phabricator via cfe-commits]

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