[clang] Refactor target attribute mangling. #81893

labrinea · 2024-02-15T18:17:51Z

Before this patch all of the 'target', 'target_version' and 'target_clones' attributes were sharing a common mangling logic across different targets. However we would like to differenciate this logic, therefore I have moved the default path to ABIInfo and provided overrides for AArch64. This way we can resolve feature aliases without affecting the name mangling. The PR #80540 demonstrates a motivating case.

llvmbot · 2024-02-15T18:18:21Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang-codegen

Author: Alexandros Lamprineas (labrinea)

Changes

Before this patch all of the 'target', 'target_version' and 'target_clones' attributes were sharing a common mangling logic across different targets. However we would like to differenciate this logic, therefore I have moved the default path to TargetInfo and provided overrides for AArch64. This way we can resolve feature aliases without affecting the name mangling The PR #80540 demonstrates a motivating case.

Full diff: https://github.com/llvm/llvm-project/pull/81893.diff

5 Files Affected:

(modified) clang/include/clang/Basic/TargetInfo.h (+8)
(modified) clang/lib/Basic/TargetInfo.cpp (+51)
(modified) clang/lib/Basic/Targets/AArch64.cpp (+27)
(modified) clang/lib/Basic/Targets/AArch64.h (+7)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+13-95)

diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 2f80234c0dc7e3..7ea4e0c0197761 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -14,6 +14,7 @@
 #ifndef LLVM_CLANG_BASIC_TARGETINFO_H
 #define LLVM_CLANG_BASIC_TARGETINFO_H
 
+#include "clang/AST/Attr.h"
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/BitmaskEnum.h"
 #include "clang/Basic/CodeGenOptions.h"
@@ -1321,6 +1322,13 @@ class TargetInfo : public TransferrableTargetInfo,
     return isValidCPUName(Name);
   }
 
+  virtual std::string getManglingSuffixFromAttr(TargetAttr *Attr) const;
+  virtual std::string getManglingSuffixFromAttr(TargetVersionAttr *Attr) const;
+  virtual std::string getManglingSuffixFromAttr(TargetClonesAttr *Attr,
+                                                unsigned VersionIndex) const;
+
+  virtual std::string getManglingSuffixFromStr(StringRef AttrStr) const;
+
   virtual ParsedTargetAttr parseTargetAttr(StringRef Str) const;
 
   /// Determine whether this TargetInfo supports tune in target attribute.
diff --git a/clang/lib/Basic/TargetInfo.cpp b/clang/lib/Basic/TargetInfo.cpp
index 08fb0f2bb1bad6..c17fd4949ef0cd 100644
--- a/clang/lib/Basic/TargetInfo.cpp
+++ b/clang/lib/Basic/TargetInfo.cpp
@@ -535,6 +535,57 @@ bool TargetInfo::initFeatureMap(
   return true;
 }
 
+std::string TargetInfo::getManglingSuffixFromAttr(TargetAttr *Attr) const {
+  if (Attr->isDefaultVersion())
+    return {};
+
+  return getManglingSuffixFromStr(Attr->getFeaturesStr());
+}
+
+std::string
+TargetInfo::getManglingSuffixFromAttr(TargetVersionAttr *Attr) const {
+  return getManglingSuffixFromStr(Attr->getNamesStr());
+}
+
+std::string TargetInfo::getManglingSuffixFromAttr(TargetClonesAttr *Attr,
+                                                  unsigned Index) const {
+  std::string Suffix = getManglingSuffixFromStr(Attr->getFeatureStr(Index));
+  Suffix.append("." + Twine(Attr->getMangledIndex(Index)).str());
+  return Suffix;
+}
+
+std::string TargetInfo::getManglingSuffixFromStr(StringRef AttrStr) const {
+  if (AttrStr == "default")
+    return ".default";
+
+  std::string ManglingSuffix(".");
+  ParsedTargetAttr Info = parseTargetAttr(AttrStr);
+
+  llvm::sort(Info.Features, [this](StringRef LHS, StringRef RHS) {
+    // Multiversioning doesn't allow "no-${feature}", so we can
+    // only have "+" prefixes here.
+    assert(LHS.starts_with("+") && RHS.starts_with("+") &&
+           "Features should always have a prefix.");
+    return multiVersionSortPriority(LHS.substr(1)) >
+           multiVersionSortPriority(RHS.substr(1));
+   });
+
+   bool IsFirst = true;
+   if (!Info.CPU.empty()) {
+     IsFirst = false;
+     ManglingSuffix.append(Twine("arch_", Info.CPU).str());
+   }
+
+   for (StringRef Feat : Info.Features) {
+     if (!IsFirst)
+       ManglingSuffix.append("_");
+     IsFirst = false;
+     ManglingSuffix.append(Feat.substr(1).str());
+   }
+
+   return ManglingSuffix;
+}
+
 ParsedTargetAttr TargetInfo::parseTargetAttr(StringRef Features) const {
   ParsedTargetAttr Ret;
   if (Features == "default")
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index c49461bd20eeec..d288c431a46207 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1105,6 +1105,33 @@ bool AArch64TargetInfo::initFeatureMap(
   return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
 }
 
+std::string AArch64TargetInfo::getManglingSuffixFromAttr(TargetClonesAttr *Attr,
+                                                         unsigned Index) const {
+  return getManglingSuffixFromStr(Attr->getFeatureStr(Index));
+}
+
+std::string AArch64TargetInfo::getManglingSuffixFromStr(StringRef Str) const {
+  if (Str == "default")
+    return ".default";
+
+  std::string ManglingSuffix("._");
+  SmallVector<StringRef, 8> Features;
+  Str.split(Features, "+");
+  for (auto &Feat : Features)
+    Feat = Feat.trim();
+
+  // TODO Lexicographical order won't break the ABI if priorities change.
+  llvm::sort(Features, [this](const StringRef LHS, const StringRef RHS) {
+    return multiVersionSortPriority(LHS) < multiVersionSortPriority(RHS);
+  });
+
+  for (auto &Feat : Features)
+    if (auto Ext = llvm::AArch64::parseArchExtension(Feat))
+      ManglingSuffix.append(Twine("M", Ext->Name).str());
+
+  return ManglingSuffix;
+}
+
 // Parse AArch64 Target attributes, which are a comma separated list of:
 //  "arch=<arch>" - parsed to features as per -march=..
 //  "cpu=<cpu>" - parsed to features as per -mcpu=.., with CPU set to <cpu>
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index f4ec3543f4082c..d5b09e064660e9 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -164,7 +164,14 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
                          bool Enabled) const override;
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
+
+  using TargetInfo::getManglingSuffixFromAttr;
+  std::string getManglingSuffixFromAttr(TargetClonesAttr *Attr,
+                                        unsigned VersionIndex) const override;
+  std::string getManglingSuffixFromStr(StringRef Str) const override;
+
   ParsedTargetAttr parseTargetAttr(StringRef Str) const override;
+
   bool supportsTargetAttributeTune() const override { return true; }
 
   bool checkArithmeticFenceSupported() const override { return true; }
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 64836f8d174710..cc6e9f206f067f 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -1724,59 +1724,6 @@ static void AppendCPUSpecificCPUDispatchMangling(const CodeGenModule &CGM,
     Out << ".resolver";
 }
 
-static void AppendTargetVersionMangling(const CodeGenModule &CGM,
-                                        const TargetVersionAttr *Attr,
-                                        raw_ostream &Out) {
-  if (Attr->isDefaultVersion()) {
-    Out << ".default";
-    return;
-  }
-  Out << "._";
-  const TargetInfo &TI = CGM.getTarget();
-  llvm::SmallVector<StringRef, 8> Feats;
-  Attr->getFeatures(Feats);
-  llvm::stable_sort(Feats, [&TI](const StringRef FeatL, const StringRef FeatR) {
-    return TI.multiVersionSortPriority(FeatL) <
-           TI.multiVersionSortPriority(FeatR);
-  });
-  for (const auto &Feat : Feats) {
-    Out << 'M';
-    Out << Feat;
-  }
-}
-
-static void AppendTargetMangling(const CodeGenModule &CGM,
-                                 const TargetAttr *Attr, raw_ostream &Out) {
-  if (Attr->isDefaultVersion())
-    return;
-
-  Out << '.';
-  const TargetInfo &Target = CGM.getTarget();
-  ParsedTargetAttr Info = Target.parseTargetAttr(Attr->getFeaturesStr());
-  llvm::sort(Info.Features, [&Target](StringRef LHS, StringRef RHS) {
-    // Multiversioning doesn't allow "no-${feature}", so we can
-    // only have "+" prefixes here.
-    assert(LHS.starts_with("+") && RHS.starts_with("+") &&
-           "Features should always have a prefix.");
-    return Target.multiVersionSortPriority(LHS.substr(1)) >
-           Target.multiVersionSortPriority(RHS.substr(1));
-  });
-
-  bool IsFirst = true;
-
-  if (!Info.CPU.empty()) {
-    IsFirst = false;
-    Out << "arch_" << Info.CPU;
-  }
-
-  for (StringRef Feat : Info.Features) {
-    if (!IsFirst)
-      Out << '_';
-    IsFirst = false;
-    Out << Feat.substr(1);
-  }
-}
-
 // Returns true if GD is a function decl with internal linkage and
 // needs a unique suffix after the mangled name.
 static bool isUniqueInternalLinkageDecl(GlobalDecl GD,
@@ -1786,41 +1733,6 @@ static bool isUniqueInternalLinkageDecl(GlobalDecl GD,
          (CGM.getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage);
 }
 
-static void AppendTargetClonesMangling(const CodeGenModule &CGM,
-                                       const TargetClonesAttr *Attr,
-                                       unsigned VersionIndex,
-                                       raw_ostream &Out) {
-  const TargetInfo &TI = CGM.getTarget();
-  if (TI.getTriple().isAArch64()) {
-    StringRef FeatureStr = Attr->getFeatureStr(VersionIndex);
-    if (FeatureStr == "default") {
-      Out << ".default";
-      return;
-    }
-    Out << "._";
-    SmallVector<StringRef, 8> Features;
-    FeatureStr.split(Features, "+");
-    llvm::stable_sort(Features,
-                      [&TI](const StringRef FeatL, const StringRef FeatR) {
-                        return TI.multiVersionSortPriority(FeatL) <
-                               TI.multiVersionSortPriority(FeatR);
-                      });
-    for (auto &Feat : Features) {
-      Out << 'M';
-      Out << Feat;
-    }
-  } else {
-    Out << '.';
-    StringRef FeatureStr = Attr->getFeatureStr(VersionIndex);
-    if (FeatureStr.starts_with("arch="))
-      Out << "arch_" << FeatureStr.substr(sizeof("arch=") - 1);
-    else
-      Out << FeatureStr;
-
-    Out << '.' << Attr->getMangledIndex(VersionIndex);
-  }
-}
-
 static std::string getMangledNameImpl(CodeGenModule &CGM, GlobalDecl GD,
                                       const NamedDecl *ND,
                                       bool OmitMultiVersionMangling = false) {
@@ -1874,16 +1786,22 @@ static std::string getMangledNameImpl(CodeGenModule &CGM, GlobalDecl GD,
                                              FD->getAttr<CPUSpecificAttr>(),
                                              GD.getMultiVersionIndex(), Out);
         break;
-      case MultiVersionKind::Target:
-        AppendTargetMangling(CGM, FD->getAttr<TargetAttr>(), Out);
+      case MultiVersionKind::Target: {
+        auto *Attr = FD->getAttr<TargetAttr>();
+        Out << CGM.getTarget().getManglingSuffixFromAttr(Attr);
         break;
-      case MultiVersionKind::TargetVersion:
-        AppendTargetVersionMangling(CGM, FD->getAttr<TargetVersionAttr>(), Out);
+      }
+      case MultiVersionKind::TargetVersion: {
+        auto *Attr = FD->getAttr<TargetVersionAttr>();
+        Out << CGM.getTarget().getManglingSuffixFromAttr(Attr);
         break;
-      case MultiVersionKind::TargetClones:
-        AppendTargetClonesMangling(CGM, FD->getAttr<TargetClonesAttr>(),
-                                   GD.getMultiVersionIndex(), Out);
+      }
+      case MultiVersionKind::TargetClones: {
+        auto *Attr = FD->getAttr<TargetClonesAttr>();
+        unsigned VersionIndex = GD.getMultiVersionIndex();
+        Out << CGM.getTarget().getManglingSuffixFromAttr(Attr, VersionIndex);
         break;
+      }
       case MultiVersionKind::None:
         llvm_unreachable("None multiversion type isn't valid here");
       }

github-actions · 2024-02-15T18:20:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

erichkeane

I wonder if this should be a part of the Targets in CodeGen instead of here?

clang/include/clang/Basic/TargetInfo.h

erichkeane · 2024-02-15T18:23:48Z

I wonder if this should be a part of the Targets in CodeGen instead of here?

Hrm, this is a bad idea as the rest of mangling is in AST, but having mangling outside of AST seems to be the problem here.

labrinea · 2024-02-16T12:20:08Z

I wonder if this should be a part of the Targets in CodeGen instead of here?

Hrm, this is a bad idea as the rest of mangling is in AST, but having mangling outside of AST seems to be the problem here.

I could move the logic under ABIInfo which is already referencing AST. Overrides of ABIInfo are provided in CodeGen/Targets as you suggested. It doesn't seem a bad idea to me.

labrinea · 2024-02-27T08:08:11Z

ping

erichkeane

Hi-
Sorry for the delay, I didn't see the updated commit, so thanks for the ping.

In general this is 'about right', but I don't like the 'getManglingSuffix' type of thing. I believe we should have these functions take an ostream and append to it, that way we save the copies that end up coming from trucking std::strings all around.

clang/lib/CodeGen/Targets/AArch64.cpp

Before this patch all of the 'target', 'target_version' and 'target_clones' attributes were sharing a common mangling logic across different targets. However we would like to differenciate this logic, therefore I have moved the default path to ABIInfo and provided overrides for AArch64. This way we can resolve feature aliases without affecting the name mangling The PR llvm#80540 demonstrates a motivating case.

Before this patch all of the 'target', 'target_version' and 'target_clones' attributes were sharing a common mangling logic across different targets. However we would like to differenciate this logic, therefore I have moved the default path to ABIInfo and provided overrides for AArch64. This way we can resolve feature aliases without affecting the name mangling. The PR llvm#80540 demonstrates a motivating case. Cherry-pick: b42b7c8

Before this patch all of the 'target', 'target_version' and 'target_clones' attributes were sharing a common mangling logic across different targets. However we would like to differenciate this logic, therefore I have moved the default path to ABIInfo and provided overrides for AArch64. This way we can resolve feature aliases without affecting the name mangling. The PR llvm#80540 demonstrates a motivating case.

labrinea requested review from jroelofs, erichkeane, davemgreen and ilinpv February 15, 2024 18:17

llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Feb 15, 2024

erichkeane reviewed Feb 15, 2024

View reviewed changes

clang/include/clang/Basic/TargetInfo.h Outdated Show resolved Hide resolved

labrinea force-pushed the refactor-target-attribute-mangling branch from 81ff939 to 20734a1 Compare February 26, 2024 10:29

erichkeane reviewed Feb 27, 2024

View reviewed changes

labrinea force-pushed the refactor-target-attribute-mangling branch 2 times, most recently from 4bff4f1 to 2b16239 Compare February 28, 2024 12:00

erichkeane reviewed Feb 28, 2024

View reviewed changes

clang/lib/CodeGen/Targets/AArch64.cpp Outdated Show resolved Hide resolved

labrinea force-pushed the refactor-target-attribute-mangling branch from 2b16239 to ed8c2af Compare February 28, 2024 15:48

erichkeane approved these changes Feb 28, 2024

View reviewed changes

labrinea merged commit b42b7c8 into llvm:main Feb 28, 2024

labrinea deleted the refactor-target-attribute-mangling branch February 28, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Refactor target attribute mangling. #81893

[clang] Refactor target attribute mangling. #81893

Uh oh!

labrinea commented Feb 15, 2024 •

edited

Loading

Uh oh!

llvmbot commented Feb 15, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 15, 2024 •

edited

Loading

Uh oh!

erichkeane left a comment

Uh oh!

Uh oh!

erichkeane commented Feb 15, 2024

Uh oh!

labrinea commented Feb 16, 2024

Uh oh!

labrinea commented Feb 27, 2024

Uh oh!

erichkeane left a comment

Uh oh!

Uh oh!

Uh oh!

[clang] Refactor target attribute mangling. #81893

[clang] Refactor target attribute mangling. #81893

Uh oh!

Conversation

labrinea commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane commented Feb 15, 2024

Uh oh!

labrinea commented Feb 16, 2024

Uh oh!

labrinea commented Feb 27, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

labrinea commented Feb 15, 2024 •

edited

Loading

llvmbot commented Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading