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@+1 {{version list contains no code impact entries}}
> void __attribute__((target_clones("sha1+pmull"))) warn2(void);
>
> // expected-warning@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
