Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][Clang][CodeGen] Improve performance for vtable metadata generation #67066

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Sep 21, 2023

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels Sep 21, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 21, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Changes

This is stacked on top of #66816
Disregard first commit.


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

12 Files Affected:

  • (modified) clang/include/clang/AST/Mangle.h (+2-2)
  • (modified) clang/lib/AST/Expr.cpp (+1-1)
  • (modified) clang/lib/AST/ItaniumMangle.cpp (+4-4)
  • (modified) clang/lib/AST/MicrosoftMangle.cpp (+4-4)
  • (modified) clang/lib/CodeGen/CGBlocks.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CGVTables.cpp (+22-31)
  • (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+1-1)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+2-2)
  • (modified) clang/lib/CodeGen/CodeGenTBAA.cpp (+2-2)
  • (modified) clang/test/CodeGen/ubsan-function-sugared.cpp (+1-2)
  • (modified) clang/unittests/AST/DeclTest.cpp (+2-2)
diff --git a/clang/include/clang/AST/Mangle.h b/clang/include/clang/AST/Mangle.h
index c04bcc7f01cb4e6..e586b0cec43dfd6 100644
--- a/clang/include/clang/AST/Mangle.h
+++ b/clang/include/clang/AST/Mangle.h
@@ -178,8 +178,8 @@ class MangleContext {
   /// or type uniquing.
   /// TODO: Extend this to internal types by generating names that are unique
   /// across translation units so it can be used with LTO.
-  virtual void mangleTypeName(QualType T, raw_ostream &,
-                              bool NormalizeIntegers = false) = 0;
+  virtual void mangleCanonicalTypeName(QualType T, raw_ostream &,
+                                       bool NormalizeIntegers = false) = 0;
 
   /// @}
 };
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 4f3837371b3fc5e..f94482206b738d5 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -659,7 +659,7 @@ std::string SYCLUniqueStableNameExpr::ComputeName(ASTContext &Context,
   std::string Buffer;
   Buffer.reserve(128);
   llvm::raw_string_ostream Out(Buffer);
-  Ctx->mangleTypeName(Ty, Out);
+  Ctx->mangleCanonicalTypeName(Ty, Out);
 
   return Out.str();
 }
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 3bbc0a5767ff49d..87c3f233ddd7bfd 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -112,8 +112,8 @@ class ItaniumMangleContextImpl : public ItaniumMangleContext {
   void mangleCXXRTTI(QualType T, raw_ostream &) override;
   void mangleCXXRTTIName(QualType T, raw_ostream &,
                          bool NormalizeIntegers) override;
-  void mangleTypeName(QualType T, raw_ostream &,
-                      bool NormalizeIntegers) override;
+  void mangleCanonicalTypeName(QualType T, raw_ostream &,
+                               bool NormalizeIntegers) override;
 
   void mangleCXXCtorComdat(const CXXConstructorDecl *D, raw_ostream &) override;
   void mangleCXXDtorComdat(const CXXDestructorDecl *D, raw_ostream &) override;
@@ -7041,8 +7041,8 @@ void ItaniumMangleContextImpl::mangleCXXRTTIName(
   Mangler.mangleType(Ty);
 }
 
-void ItaniumMangleContextImpl::mangleTypeName(QualType Ty, raw_ostream &Out,
-                                              bool NormalizeIntegers = false) {
+void ItaniumMangleContextImpl::mangleCanonicalTypeName(
+    QualType Ty, raw_ostream &Out, bool NormalizeIntegers = false) {
   mangleCXXRTTIName(Ty, Out, NormalizeIntegers);
 }
 
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index 79175c79de96bf8..c1320cbdf0e6385 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -196,8 +196,8 @@ class MicrosoftMangleContextImpl : public MicrosoftMangleContext {
   mangleCXXRTTICompleteObjectLocator(const CXXRecordDecl *Derived,
                                      ArrayRef<const CXXRecordDecl *> BasePath,
                                      raw_ostream &Out) override;
-  void mangleTypeName(QualType T, raw_ostream &,
-                      bool NormalizeIntegers) override;
+  void mangleCanonicalTypeName(QualType T, raw_ostream &,
+                               bool NormalizeIntegers) override;
   void mangleReferenceTemporary(const VarDecl *, unsigned ManglingNumber,
                                 raw_ostream &) override;
   void mangleStaticGuardVariable(const VarDecl *D, raw_ostream &Out) override;
@@ -3838,13 +3838,13 @@ void MicrosoftMangleContextImpl::mangleSEHFinallyBlock(
   Mangler.mangleName(EnclosingDecl);
 }
 
-void MicrosoftMangleContextImpl::mangleTypeName(
+void MicrosoftMangleContextImpl::mangleCanonicalTypeName(
     QualType T, raw_ostream &Out, bool NormalizeIntegers = false) {
   // This is just a made up unique string for the purposes of tbaa.  undname
   // does *not* know how to demangle it.
   MicrosoftCXXNameMangler Mangler(*this, Out);
   Mangler.getStream() << '?';
-  Mangler.mangleType(T, SourceRange());
+  Mangler.mangleType(T.getCanonicalType(), SourceRange());
 }
 
 void MicrosoftMangleContextImpl::mangleReferenceTemporary(
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 4f64012fc1a5c39..afa9156ceaffbbd 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -1716,7 +1716,7 @@ static std::string getBlockCaptureStr(const CGBlockInfo::Capture &Cap,
     Str += "c";
     SmallString<256> TyStr;
     llvm::raw_svector_ostream Out(TyStr);
-    CGM.getCXXABI().getMangleContext().mangleTypeName(CaptureTy, Out);
+    CGM.getCXXABI().getMangleContext().mangleCanonicalTypeName(CaptureTy, Out);
     Str += llvm::to_string(TyStr.size()) + TyStr.c_str();
     break;
   }
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 92b7c8d4aa546f0..c5096201ad95353 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -9069,7 +9069,7 @@ void CGOpenMPRuntime::emitUserDefinedMapper(const OMPDeclareMapperDecl *D,
   llvm::FunctionType *FnTy = CGM.getTypes().GetFunctionType(FnInfo);
   SmallString<64> TyStr;
   llvm::raw_svector_ostream Out(TyStr);
-  CGM.getCXXABI().getMangleContext().mangleTypeName(Ty, Out);
+  CGM.getCXXABI().getMangleContext().mangleCanonicalTypeName(Ty, Out);
   std::string Name = getName({"omp_mapper", TyStr, D->getName()});
   auto *Fn = llvm::Function::Create(FnTy, llvm::GlobalValue::InternalLinkage,
                                     Name, &CGM.getModule());
diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 23cfcdd138439f0..54df3dd4297a804 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Transforms/Utils/Cloning.h"
 #include <algorithm>
 #include <cstdio>
+#include <utility>
 
 using namespace clang;
 using namespace CodeGen;
@@ -1308,43 +1309,33 @@ void CodeGenModule::EmitVTableTypeMetadata(const CXXRecordDecl *RD,
 
   CharUnits ComponentWidth = GetTargetTypeStoreSize(getVTableComponentType());
 
-  typedef std::pair<const CXXRecordDecl *, unsigned> AddressPoint;
+  struct AddressPoint {
+    const CXXRecordDecl *Base;
+    size_t Offset;
+    std::string TypeName;
+    bool operator<(const AddressPoint &RHS) const {
+      int D = TypeName.compare(RHS.TypeName);
+      return D < 0 || (D == 0 && Offset < RHS.Offset);
+    }
+  };
   std::vector<AddressPoint> AddressPoints;
-  for (auto &&AP : VTLayout.getAddressPoints())
-    AddressPoints.push_back(std::make_pair(
-        AP.first.getBase(), VTLayout.getVTableOffset(AP.second.VTableIndex) +
-                                AP.second.AddressPointIndex));
+  for (auto &&AP : VTLayout.getAddressPoints()) {
+    AddressPoint N{AP.first.getBase(),
+                   VTLayout.getVTableOffset(AP.second.VTableIndex) +
+                       AP.second.AddressPointIndex};
+    llvm::raw_string_ostream Stream(N.TypeName);
+    getCXXABI().getMangleContext().mangleCanonicalTypeName(
+        QualType(N.Base->getTypeForDecl(), 0), Stream);
+    AddressPoints.push_back(std::move(N));
+  }
 
   // Sort the address points for determinism.
-  llvm::sort(AddressPoints, [this](const AddressPoint &AP1,
-                                   const AddressPoint &AP2) {
-    if (&AP1 == &AP2)
-      return false;
-
-    std::string S1;
-    llvm::raw_string_ostream O1(S1);
-    getCXXABI().getMangleContext().mangleTypeName(
-        QualType(AP1.first->getTypeForDecl(), 0), O1);
-    O1.flush();
-
-    std::string S2;
-    llvm::raw_string_ostream O2(S2);
-    getCXXABI().getMangleContext().mangleTypeName(
-        QualType(AP2.first->getTypeForDecl(), 0), O2);
-    O2.flush();
-
-    if (S1 < S2)
-      return true;
-    if (S1 != S2)
-      return false;
-
-    return AP1.second < AP2.second;
-  });
+  llvm::sort(AddressPoints);
 
   ArrayRef<VTableComponent> Comps = VTLayout.vtable_components();
   for (auto AP : AddressPoints) {
     // Create type metadata for the address point.
-    AddVTableTypeMetadata(VTable, ComponentWidth * AP.second, AP.first);
+    AddVTableTypeMetadata(VTable, ComponentWidth * AP.Offset, AP.Base);
 
     // The class associated with each address point could also potentially be
     // used for indirect calls via a member function pointer, so we need to
@@ -1356,7 +1347,7 @@ void CodeGenModule::EmitVTableTypeMetadata(const CXXRecordDecl *RD,
       llvm::Metadata *MD = CreateMetadataIdentifierForVirtualMemPtrType(
           Context.getMemberPointerType(
               Comps[I].getFunctionDecl()->getType(),
-              Context.getRecordType(AP.first).getTypePtr()));
+              Context.getRecordType(AP.Base).getTypePtr()));
       VTable->addTypeMetadata((ComponentWidth * I).getQuantity(), MD);
     }
   }
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index bf83171e2c68146..b76ca56d1c3940b 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -576,7 +576,7 @@ CodeGenFunction::getUBSanFunctionTypeHash(QualType Ty) const {
     Ty = getContext().getFunctionTypeWithExceptionSpec(Ty, EST_None);
   std::string Mangled;
   llvm::raw_string_ostream Out(Mangled);
-  CGM.getCXXABI().getMangleContext().mangleTypeName(Ty, Out, false);
+  CGM.getCXXABI().getMangleContext().mangleCanonicalTypeName(Ty, Out, false);
   return llvm::ConstantInt::get(
       CGM.Int32Ty, static_cast<uint32_t>(llvm::xxh3_64bits(Mangled)));
 }
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 8b0c9340775cbe9..da0e471c8d06e3e 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -1999,7 +1999,7 @@ llvm::ConstantInt *CodeGenModule::CreateKCFITypeId(QualType T) {
 
   std::string OutName;
   llvm::raw_string_ostream Out(OutName);
-  getCXXABI().getMangleContext().mangleTypeName(
+  getCXXABI().getMangleContext().mangleCanonicalTypeName(
       T, Out, getCodeGenOpts().SanitizeCfiICallNormalizeIntegers);
 
   if (getCodeGenOpts().SanitizeCfiICallNormalizeIntegers)
@@ -7199,7 +7199,7 @@ CodeGenModule::CreateMetadataIdentifierImpl(QualType T, MetadataTypeMap &Map,
   if (isExternallyVisible(T->getLinkage())) {
     std::string OutName;
     llvm::raw_string_ostream Out(OutName);
-    getCXXABI().getMangleContext().mangleTypeName(
+    getCXXABI().getMangleContext().mangleCanonicalTypeName(
         T, Out, getCodeGenOpts().SanitizeCfiICallNormalizeIntegers);
 
     if (getCodeGenOpts().SanitizeCfiICallNormalizeIntegers)
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index 395ed7b1d703a5e..8705d3d65f1a573 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -205,7 +205,7 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) {
 
     SmallString<256> OutName;
     llvm::raw_svector_ostream Out(OutName);
-    MContext.mangleTypeName(QualType(ETy, 0), Out);
+    MContext.mangleCanonicalTypeName(QualType(ETy, 0), Out);
     return createScalarTypeNode(OutName, getChar(), Size);
   }
 
@@ -391,7 +391,7 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
     if (Features.CPlusPlus) {
       // Don't use the mangler for C code.
       llvm::raw_svector_ostream Out(OutName);
-      MContext.mangleTypeName(QualType(Ty, 0), Out);
+      MContext.mangleCanonicalTypeName(QualType(Ty, 0), Out);
     } else {
       OutName = RD->getName();
     }
diff --git a/clang/test/CodeGen/ubsan-function-sugared.cpp b/clang/test/CodeGen/ubsan-function-sugared.cpp
index 238bf31f4aba6d1..fb2487c024ba903 100644
--- a/clang/test/CodeGen/ubsan-function-sugared.cpp
+++ b/clang/test/CodeGen/ubsan-function-sugared.cpp
@@ -40,5 +40,4 @@ void caller() {
 }
 
 // GNU:  ![[FUNCSAN]] = !{i32 -1056584962, i32 905068220}
-// FIXME: Wrong hash
-// MSVC: ![[FUNCSAN]] = !{i32 -1056584962, i32 165986058}
+// MSVC: ![[FUNCSAN]] = !{i32 -1056584962, i32 -1600339357}
diff --git a/clang/unittests/AST/DeclTest.cpp b/clang/unittests/AST/DeclTest.cpp
index d2977b0cb55b64a..474d3dadb5a52c6 100644
--- a/clang/unittests/AST/DeclTest.cpp
+++ b/clang/unittests/AST/DeclTest.cpp
@@ -135,8 +135,8 @@ TEST(Decl, MangleDependentSizedArray) {
   std::unique_ptr<ItaniumMangleContext> MC(
       ItaniumMangleContext::create(Ctx, Diags));
 
-  MC->mangleTypeName(DeclA->getType(), OS_A);
-  MC->mangleTypeName(DeclB->getType(), OS_B);
+  MC->mangleCanonicalTypeName(DeclA->getType(), OS_A);
+  MC->mangleCanonicalTypeName(DeclB->getType(), OS_B);
 
   ASSERT_TRUE(0 == MangleA.compare("_ZTSA_i"));
   ASSERT_TRUE(0 == MangleB.compare("_ZTSAT0__T_"));

@mizvekov mizvekov changed the title Improve sort mangled [NFC][Clang][CodeGen] Improve performance for vtable metadata generation Sep 21, 2023
@mizvekov mizvekov self-assigned this Sep 21, 2023
Mangle each AddressPoint once, instead of once per comparison.
@mizvekov mizvekov merged commit 22d8f1d into llvm:main Oct 2, 2023
3 checks passed
@mizvekov mizvekov deleted the improve_sort_mangled branch October 2, 2023 22:45
@shafik
Copy link
Collaborator

shafik commented Oct 2, 2023

Please next time before you commit add a more detailed description of the change so that readers of git log can get a better understanding of the change w/o having to view it in detail.

@mizvekov
Copy link
Contributor Author

mizvekov commented Oct 2, 2023

Weird, the commit in the MR has a description, but it lost the description when merging through the github interface.

kstoimenov added a commit that referenced this pull request Oct 3, 2023
@mizvekov
Copy link
Contributor Author

mizvekov commented Oct 3, 2023

@kstoimenov I believe you are supposed to notify the MR in case of reverts, otherwise I could have missed this if I wasn't awake.

I believe in this case the bot is misconfigured, it's using -Werror,-Wmissing-field-initializers, the 'error' is just a missing initializer, which I didn't add because I think it makes the intent clearer.

mizvekov added a commit that referenced this pull request Oct 3, 2023
…ion (#67066)

Mangle each AddressPoint once, instead of once per comparison.
@kstoimenov
Copy link
Contributor

kstoimenov commented Oct 3, 2023 via email

@mizvekov
Copy link
Contributor Author

mizvekov commented Oct 3, 2023

Could you please send me a link which describes that process? I assume that by MR you mean merge request? Thanks, Kirill

Sure.

https://github.com/llvm/llvm-project/blob/main/llvm/docs/DeveloperPolicy.rst#id62

See the section about What are the expectations around a revert?, specifically fourth bullet.

But use your best judgement here: If you revert a commit without engaging the original author, he might have a hard time figuring out there was even a revert.
And also, you need to engage for other reasons as described in that section.

@vitalybuka
Copy link
Collaborator

There is automatic github notification about revert, I assume something in the email as well.
image

What is the value of another message?

@efriedma-quic
Copy link
Collaborator

I didn't get an an email notification for that.

@vitalybuka
Copy link
Collaborator

I didn't get an an email notification for that.

Thanks, good to know.
I like that in Phabricator it was possible just reopen review making revert very visible to the author.

@kstoimenov
Copy link
Contributor

kstoimenov commented Oct 3, 2023 via email

@efriedma-quic
Copy link
Collaborator

Maybe start a discourse thread to get more eyes on this discussion? We might be able to use a hook to make llvmbot do something automatically.

Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Local branch amd-gfx 01e4b32 Merged main:824b1677a44e into amd-gfx:99c3c2f02c0f
Remote branch main e897014 [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (llvm#67066)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants