[PATCH] D127812: [AArch64] Function multiversioning support added.
Sam Tebbs via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 28 03:18:19 PDT 2022
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
More information about the cfe-commits
mailing list