[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1809
       bool DuplicateArchitecture = false;
+      bool operator==(const ParsedTargetAttr &Other) {
+        return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
This function should be marked `const`.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9333-9335
+  "multiversion function would have identical mangling to a previous "
+  "definition.  Duplicate declarations must have identical target attribute "
+  "values">;
----------------
Diagnostics are not complete sentences, so this should be reworded to be even less grammatically correct. ;-)


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9339-9340
+def err_multiversion_no_other_attrs : Error<
+  "attribute 'target' multiversioning cannot be combined with other "
+  "attributes">;
+def err_multiversion_diff : Error<
----------------
This worries me slightly. Is there a benefit to prohibiting this attribute with any other attribute? For instance, I'm thinking about a multiversioned noreturn function or a multiversioned function with a calling convention as plausible use cases.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9349
+def err_multiversion_not_allowed_on_main : Error<
+  "function multiversion not permitted on 'main'">;
+
----------------
How about `'main' cannot be a multiversion function`?


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2307-2308
+    SmallVector<StringRef, 8> FeatureList;
+    std::for_each(std::begin(RO.ParsedAttribute.Features),
+                  std::end(RO.ParsedAttribute.Features),
+                  [&FeatureList](const std::string &Feature) {
----------------
You can use `llvm::for_each` instead.


================
Comment at: lib/CodeGen/CodeGenFunction.h:3941
+        Priority = std::max(Priority, TargInfo.multiVersionSortPriority(
+                                          StringRef{Feat}.substr(1)));
+
----------------
`Feat` is already a `StringRef`?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:741
+  const auto &Target = CGM.getTarget();
+  auto CmpFunc = [&Target](StringRef LHS, StringRef RHS) {
+    return Target.multiVersionSortPriority(LHS) >
----------------
Might as well sink this into the call to `std::sort()`.


================
Comment at: lib/Sema/SemaDecl.cpp:9298
+  TargetAttr::ParsedTargetAttr ParseInfo = TA->parse();
+  const auto &TargetInfo = S.Context.getTargetInfo();
+  enum ErrType { Feature = 0, Architecture = 1 };
----------------
Don't use `auto` here as the type is not explicitly spelled out in the initialization.


================
Comment at: lib/Sema/SemaDecl.cpp:9326
+
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl *OldFD,
+                                             const FunctionDecl *NewFD,
----------------
This function uses quite a few magic numbers that might be better expressed with named values instead.


================
Comment at: lib/Sema/SemaDecl.cpp:9362-9364
+    QualType NewQType = S.getASTContext().getCanonicalType(NewFD->getType());
+    const FunctionType *NewType = cast<FunctionType>(NewQType);
+    QualType NewReturnType = NewType->getReturnType();
----------------
Formatting is off here.


================
Comment at: lib/Sema/SemaDecl.cpp:9363
+    QualType NewQType = S.getASTContext().getCanonicalType(NewFD->getType());
+    const FunctionType *NewType = cast<FunctionType>(NewQType);
+    QualType NewReturnType = NewType->getReturnType();
----------------
Can use `const auto *` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9381
+    QualType OldQType = S.getASTContext().getCanonicalType(OldFD->getType());
+    const FunctionType *OldType = cast<FunctionType>(OldQType);
+    FunctionType::ExtInfo OldTypeInfo = OldType->getExtInfo();
----------------
Can use `const auto *` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9448
+  if (NewFD->isMain()) {
+    if (NewTA && NewTA->getFeaturesStr() == "default") {
+      S.Diag(NewFD->getLocation(), diag::err_multiversion_not_allowed_on_main);
----------------
So any other form of target attribute and feature string is fine to place on `main()`?


================
Comment at: lib/Sema/SemaDecl.cpp:9478
+
+  auto *OldFD = OldDecl->getAsFunction();
+  // Unresolved 'using' statements (the other way OldDecl can be not a function)
----------------
Do not use `auto` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9494
+  TargetAttr::ParsedTargetAttr NewParsed = NewTA->parse();
+  // sort order doesn't matter, it just needs to be consistent.
+  std::sort(NewParsed.Features.begin(), NewParsed.Features.end());
----------------
Capitalization.


================
Comment at: lib/Sema/SemaDecl.cpp:9562
+  for (NamedDecl *ND : Previous) {
+    auto *CurFD = ND->getAsFunction();
+    if (!CurFD)
----------------
Do not use `auto` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9577
+    TargetAttr::ParsedTargetAttr CurParsed = CurTA->parse();
+    std::sort(CurParsed.Features.begin(), CurParsed.Features.end());
+
----------------
Given how often the features need to be sorted, would it make sense to hoist this functionality into `parse()`?


https://reviews.llvm.org/D40819