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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
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>;
----------------
ilinpv wrote:
> erichkeane wrote:
> > I'm not clear as to what this means?
> It gives a warning if target_clones attributes contains features which have no impact on code generation ( no supported yet ) and ignored. They has "<NS>" OPTION in llvm/include/llvm/Support/AArch64TargetParser.def 
> See clang/test/Sema/attr-target-clones-aarch64.c tests
> ```
> // expected-warning at +1 {{version list contains no code impact entries}}
> void __attribute__((target_clones("sha1+pmull"))) warn2(void);
> 
> // expected-warning at +1 {{version list contains no code impact entries}}
> int __attribute__((target_clones("rng", "fhm+dpb+sha1", "default"))) redecl4(void) { return 1; }
> ```
Perhaps it would be better to warn with something like "version list contains entries that don't impact code generation". I think that would be more clear.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:1311
+  /// generation.
+  virtual bool isCodeImpactFeatureName(StringRef Feature) const { return true; }
+
----------------
Similarly to the warning string, I think a name like `featureAffectsCodeGen(...)` would be more clear in its use.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:663
+    if (Feature == "-fmv") {
+      HasFMV = false;
+    }
----------------
Slight nit, but braces aren't needed here.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2561
   } else if (!TargetDecl->isMultiVersion() &&
-             TargetDecl->hasAttr<TargetAttr>()) {
+             (TargetDecl->hasAttr<TargetAttr>())) {
     // Get the required features for the callee.
----------------
Parentheses not needed.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2664
+  default:
+    assert(false && "Only implemented for x86 and AArch64 targets");
+  }
----------------
I'm not 100% sure on the differences between `assert(..)` and `LLvm_unreachable(...)`, but perhaps that would be cleaner than an assertion on `false`.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2670
+    llvm::Function *Resolver, ArrayRef<MultiVersionResolverOption> Options) {
+  assert(!Options.empty() && "No Multiversion Resolver Options Found");
+  assert(Options.back().Conditions.Features.size() == 0 &&
----------------
Nit on the capital letters for every word. Only "No" requires capitalisation.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:10497
+      (TA || TVA) &&
+      "MultiVersion Candidate requires a target or target_version attribute");
   const TargetInfo &TargetInfo = S.Context.getTargetInfo();
----------------
Candidate doesn't need capitalisation either.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3521
     bool DefaultIsDupe = false;
+    bool IsCodeImpact = false;
     if (Cur.empty())
----------------
I think `HasCodeImpact` would be more explanatory.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127812/new/

https://reviews.llvm.org/D127812