rsmith added inline comments.
================
Comment at: include/clang/Basic/Attr.td:2031-2042
+ mutable unsigned ActiveArgIndex = 0;
+ void AdvanceActiveArgIndex() const {
+ ++ActiveArgIndex;
+ while(ActiveArgIndex < featuresStrs_size()) {
+ if (std::find(featuresStrs_begin(),
+ featuresStrs_begin() + ActiveArgIndex,
+ *(featuresStrs_begin() + ActiveArgIndex))
----------------
Sorry, I don't think it's acceptable from a design perspective to have mutable
state in an `Attr` object, even if you can ensure that only one client of the
`Attr` will be using the state at a time. CodeGen is going to need to track its
own index into the attribute's list of clones.
================
Comment at: include/clang/Basic/AttrDocs.td:1619-1621
+Note that unlike the ``target`` syntax, every option listed creates a new
+version, disregarding whether it is split on a comma inside or outside a
string.
+The following will emit 4 versions of the function.
----------------
Can we warn on attributes that contain multiple strings where one or more such
strings contains a comma? That seems like it would always be user error.
I think I'd then prefer to document this as: "The versions can either be listed
as a comma-separated sequence of string literals or as a single string literal
containing a comma-separated list of versions. For compatibility with GCC, the
two formats can be mixed. For example, the following will emit 4 versions of
the function:"
================
Comment at: lib/CodeGen/CodeGenModule.cpp:961-970
if (const auto *FD = dyn_cast<FunctionDecl>(ND))
if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion())
AppendCPUSpecificCPUDispatchMangling(
CGM, FD->getAttr<CPUSpecificAttr>(), Out);
+ else if (FD->isTargetClonesMultiVersion())
+ AppendTargetClonesMangling(CGM, FD->getAttr<TargetClonesAttr>(), Out);
----------------
This chain of `if`s and similar things elsewhere make me think we're missing an
abstraction. Perhaps `FunctionDecl` should have a `getMultiVersionAttr()`
and/or `getMultiVersionKind()` functions?
================
Comment at: lib/CodeGen/CodeGenModule.cpp:1043
+
+ std::pair<GlobalDecl, unsigned> SpecCanonicalGD{CanonicalGD, VersionID};
----------------
This target index should be part of the `GlobalDecl`, not tracked alongside it,
because it identifies which global we're talking about.
================
Comment at: lib/Sema/SemaDecl.cpp:9800-9807
// Mixing Multiversioning types is prohibited.
- if ((NewTA && NewCPUDisp) || (NewTA && NewCPUSpec) ||
- (NewCPUDisp && NewCPUSpec)) {
+ if ((static_cast<bool>(NewTA) + static_cast<bool>(NewCPUDisp) +
+ static_cast<bool>(NewCPUSpec) + static_cast<bool>(NewTargetClones)) >
+ 1) {
S.Diag(NewFD->getLocation(), diag::err_multiversion_types_mixed);
NewFD->setInvalidDecl();
return true;
----------------
We should also disallow multiple `TargetClonesAttr`s on the same declaration.
GCC allows this and then ignores all but the last such attribute; we can do
better.
================
Comment at: lib/Sema/SemaDecl.cpp:9811-9812
// Main isn't allowed to become a multiversion function, however it IS
// permitted to have 'main' be marked with the 'target' optimization hint.
if (NewFD->isMain()) {
----------------
Why can't `main` be multiversioned? That seems like an arbitrary restriction.
================
Comment at: lib/Sema/SemaDecl.cpp:9839
- if (OldFD->isMultiVersion() && MVType == MultiVersioning::None) {
+ // MultiVersioned redeclarations aren't allowed to omit the attribute except
+ // for target_clones.
----------------
I would expect the plain English term to not have interior capitalization.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3011
+bool Sema::checkTargetClonesAttr(SourceLocation LiteralLoc, StringRef Str,
+ bool &HasDefault,
----------------
Should be something like `checkTargetClonesAttrStr` to indicate that this is
checking one specific string, not the entire attribute.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3018-3029
+ // Warn on empty at the beginning of a string.
+ if (Str.size() == 0 || Str[0] == ',')
+ return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+ << Unsupported << None << "" << TargetClones;
+
+ while (Str.size() != 0) {
+ // Remove the comma we found last time through.
----------------
When looping through comma-delimited strings, it's easier to use
`StringRef::split`:
```
std::pair<StringRef, StringRef> Parts = {{}, Str};
while (!Parts.second.empty()) {
Parts = Parts.second.split(',');
StringRef Cur = Parts.first.trim();
// ...
}
```
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3035-3043
+ return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+ << Unsupported << Architecture
+ << Cur.drop_front(sizeof("arch=") - 1) << TargetClones;
+ } else if (Cur == "default") {
+ HasDefault = true;
+ continue;
+ } else if (!Context.getTargetInfo().isValidFeatureName(Cur))
----------------
Rather than just repeating the problematic portion of the string textually in
the diagnostic, pass the `StringLiteral` into here and use
`StringLiteral::getLocationOfByte` to highlight the problematic source range
within the string.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3068-3071
+ if (!HasDefault) {
+ S.Diag(AL.getLoc(), diag::err_target_clone_must_have_default);
+ return;
+ }
----------------
Do we need any other validation here? What if there are duplicate versions?
================
Comment at: test/CodeGen/attr-cpuspecific.c:14-15
-__attribute__((cpu_specific(ivybridge)))
-void NotCalled(void){}
+__attribute__((cpu_specific(ivybridge))) inline void InlineSingleVersion(void)
{}
+// CHECK: define available_externally void @InlineSingleVersion.S()
#[[S:[0-9]+]]
+
----------------
Should this really have external linkage? These `.` suffixes are reserved for
vendor-specific manglings that typically should only be used for symbols with
internal linkage.
If you really want to give these symbols external linkage, I think they should
at least be put in a COMDAT keyed off the primary symbol so that we don't get a
mishmash of different suffix combinations from different compilers.
https://reviews.llvm.org/D51650
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
