[PATCH] D38596: Implement attribute target multiversioning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Decl.h:2226
+  MultiVersionKind getMultiVersionKind() const {
+    return static_cast<MultiVersionKind>(this->MultiVersion);
+  }
----------------
Drop the `this->`


================
Comment at: include/clang/AST/Decl.h:2229
+  void setMultiVersionKind(MultiVersionKind MV) {
+    this->MultiVersion = static_cast<unsigned>(MV);
+  }
----------------
Same


================
Comment at: include/clang/Basic/Attr.td:1847
+
+      bool operator ==(const ParsedTargetAttr &Other) const {
+        return std::tie(Features, Architecture) ==
----------------
`operator==` instead of `operator ==`


================
Comment at: include/clang/Basic/Attr.td:1852
+
+      bool operator !=(const ParsedTargetAttr &Other) const {
+        return !(*this == Other);
----------------
Same


================
Comment at: include/clang/Basic/Attr.td:1898
+      llvm::raw_svector_ostream OS(Buffer);
+      auto Info = parse();
+
----------------
Don't use `auto` here.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9301
+def err_target_causes_illegal_multiversioning
+    : Error<"function redeclaration causes a multiversioned function, but a "
+            "previous declaration lacks a 'target' attribute">;
----------------
'causes' seems a bit strange; how about `function redeclaration declares a multiversioned function,`? (Or something else, I'm not tied to my wording.)


================
Comment at: include/clang/Sema/Sema.h:1937
+  // Checks to see if attribute 'target' results in a valid situation.
+  bool CheckMultiVersionedDecl(FunctionDecl *NewFD, FunctionDecl *OldFD);
+  // Helper function for CheckMultiVersionDecl to ensure that a declaration
----------------
It's a bit strange that this function is called "Check" but it mutates the arguments.


================
Comment at: lib/Basic/Targets/X86.cpp:1402
+
+  for (auto &Str : TargetArray) {
+    if (Lhs == Str)
----------------
`const auto &`


================
Comment at: lib/CodeGen/CodeGenFunction.h:3707
+    llvm::Function *Function;
+    ResolverOption(TargetAttr::ParsedTargetAttr &&AttrInfo,// can be moved?
+                   llvm::Function *Function)
----------------
The comment doesn't inspire confidence with the trailing question mark. ;-)


================
Comment at: lib/Sema/SemaDecl.cpp:9241
+bool Sema::CheckMultiVersionOption(const FunctionDecl *FD) {
+  auto *TA = FD->getAttr<TargetAttr>();
+
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDecl.cpp:9245
+    return true;
+  auto ParseInfo = TA->parse();
+
----------------
Don't use `auto` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9259
+
+  for (auto &Feature : ParseInfo.Features) {
+    auto BareFeat = StringRef{Feature}.substr(1);
----------------
`const auto &`


================
Comment at: lib/Sema/SemaDecl.cpp:9281
+  // for each new architecture.
+  auto Arch = Context.getTargetInfo().getTriple().getArch();
+  if (Arch != llvm::Triple::x86_64 && Arch != llvm::Triple::x86) {
----------------
Don't use `auto` here.


================
Comment at: lib/Sema/SemaDecl.cpp:9287
+
+  bool result = true;
+  for (const auto *FD : OldFD->redecls())
----------------
Should be named `Result`


================
Comment at: lib/Sema/SemaDecl.cpp:9295
+bool Sema::CheckMultiVersionedDecl(FunctionDecl *NewFD, FunctionDecl *OldFD) {
+  // The first Decl is easy, it is either the 'All' case, or 'None'.
+  if (!OldFD) {
----------------
Remove the comma after `case`.


================
Comment at: lib/Sema/SemaDecl.cpp:9297-9300
+    if (NewFD->hasAttr<TargetAttr>())
+      NewFD->setMultiVersionKind(FunctionDecl::MultiVersionKind::All);
+    else
+      NewFD->setMultiVersionKind(FunctionDecl::MultiVersionKind::None);
----------------
Might be more succinctly written with `?:` (unsure which looks better, try it and see).


================
Comment at: lib/Sema/SemaDecl.cpp:9304
+
+  auto *TA = NewFD->getAttr<TargetAttr>();
+  switch (OldFD->getMultiVersionKind()) {
----------------
`const auto *` -- might as well hoist this to the top of the function and remove the call to `hasAttr<>()` above.


================
Comment at: lib/Sema/SemaDecl.cpp:9306
+  switch (OldFD->getMultiVersionKind()) {
+  default:
+    llvm_unreachable("Invalid State for Multiversioning.");
----------------
I suspect this will give warnings about having a `default` label in a fully-covered switch.


================
Comment at: lib/Sema/SemaDecl.cpp:9344
+    }
+    // If this doesn't convert it to a multi-version, this is clearly a 'all'
+    // case.
----------------
a 'all' -> an 'all'


================
Comment at: lib/Sema/SemaDecl.cpp:9366
+  case FunctionDecl::MultiVersionKind::Partial:
+    // A partial not adding a Target Attribute is fine, everything stays as
+    // partial.
----------------
Attribute -> attribute


================
Comment at: lib/Sema/SemaDecl.cpp:9372
+    }
+    // A partial situation is not allowed to convert to a MV, so ensure that
+    // this target doesn't cause that situation.
----------------
MV -> MultiVersion


================
Comment at: lib/Sema/SemaDecl.cpp:9524
+  // attribute.
+  if (!CheckMultiVersionedDecl(NewFD, OldDecl ? dyn_cast<FunctionDecl>(OldDecl)
+                                              : nullptr))
----------------
Can use `dyn_cast_or_null<>()` instead.


================
Comment at: lib/Sema/SemaDecl.cpp:12184
+
+  TargetAttr *NewTA = NewFD->getAttr<TargetAttr>();
+  const auto NewTAParsed = NewTA->parse();
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDecl.cpp:12185
+  TargetAttr *NewTA = NewFD->getAttr<TargetAttr>();
+  const auto NewTAParsed = NewTA->parse();
+  // Prohibit duplicate definitions.
----------------
Don't use `auto` here.


================
Comment at: lib/Sema/SemaDecl.cpp:12189
+    if (FD->isThisDeclarationADefinition()) {
+      TargetAttr *TA = FD->getAttr<TargetAttr>();
+      if (TA && !TA->isInherited() &&
----------------
`const auto *`.


https://reviews.llvm.org/D38596