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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits