[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Eli Friedman via cfe-commits]

efriedma-quic wrote:

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.

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Kirill Stoimenov via cfe-commits]

kstoimenov wrote:

Maybe one way is to add @mention which should trigger a new email message?

On Tue, Oct 3, 2023 at 12:42 PM Vitaly Buka ***@***.***>
wrote:

> 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.
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


-- 
Kirill Stoimenov | Software Engineer | ***@***.*** | +1-
650-253-6893


https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Vitaly Buka via cfe-commits]

vitalybuka wrote:

> 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.

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Eli Friedman via cfe-commits]

efriedma-quic wrote:

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

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Vitaly Buka via cfe-commits]

vitalybuka wrote:

There is automatic github notification about revert, I assume something in the 
email as well.
![image](https://github.com/llvm/llvm-project/assets/1531415/2455fb7c-f51a-4d6b-9df6-9c16b813bda1)

What is the value of another message? 

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

> Could you please send me a link which describes that process? I assume that 
> by MR you mean merge request? Thanks, Kirill
> […](#)
> On Mon, Oct 2, 2023 at 5:24 PM Matheus Izvekov ***@***.***> wrote: 
> @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. 
> — Reply to this email directly, view it on GitHub <[#67066 
> (comment)](https://github.com/llvm/llvm-project/pull/67066#issuecomment-1743967544)>,
>  or unsubscribe 
> 
>  . You are receiving this because you were mentioned.Message ID: ***@***.***>
> -- Kirill Stoimenov | Software Engineer | ***@***.*** | +1- 650-253-6893

Sure.

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

See specifically 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.

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Kirill Stoimenov via cfe-commits]

kstoimenov wrote:

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

Thanks,
Kirill

On Mon, Oct 2, 2023 at 5:24 PM Matheus Izvekov ***@***.***>
wrote:

> @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.
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


-- 
Kirill Stoimenov | Software Engineer | ***@***.*** | +1-
650-253-6893


https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

@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.

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

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

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Shafik Yaghmour via cfe-commits]

shafik wrote:

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.

https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov closed 
https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov edited 
https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/67066

>From 44d43a8e7fa5b19671bc3f56f934dfb63a632aa4 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 21 Sep 2023 22:57:27 +0200
Subject: [PATCH] [NFC][Clang][CodeGen] Improve performance for vtable metadata
 generation

Mangle each AddressPoint once, instead of once per comparison.
---
 clang/lib/CodeGen/CGVTables.cpp | 54 ++---
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index d782da2103b4c79..5ec38e0397bf4f7 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Transforms/Utils/Cloning.h"
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace CodeGen;
@@ -1308,44 +1309,33 @@ void CodeGenModule::EmitVTableTypeMetadata(const 
CXXRecordDecl *RD,
 
   CharUnits ComponentWidth = GetTargetTypeStoreSize(getVTableComponentType());
 
-  typedef std::pair AddressPoint;
+  struct AddressPoint {
+const CXXRecordDecl *Base;
+size_t Offset;
+std::string TypeName;
+bool operator<(const AddressPoint ) const {
+  int D = TypeName.compare(RHS.TypeName);
+  return D < 0 || (D == 0 && Offset < RHS.Offset);
+}
+  };
   std::vector AddressPoints;
-  for (auto & : VTLayout.getAddressPoints())
-AddressPoints.push_back(std::make_pair(
-AP.first.getBase(), VTLayout.getVTableOffset(AP.second.VTableIndex) +
-AP.second.AddressPointIndex));
-
-  // Sort the address points for determinism.
-  // FIXME: It's more efficient to mangle the types before sorting.
-  llvm::sort(AddressPoints, [this](const AddressPoint ,
-   const AddressPoint ) {
-if ( == )
-  return false;
-
-std::string S1;
-llvm::raw_string_ostream O1(S1);
-getCXXABI().getMangleContext().mangleCanonicalTypeName(
-QualType(AP1.first->getTypeForDecl(), 0), O1);
-O1.flush();
-
-std::string S2;
-llvm::raw_string_ostream O2(S2);
+  for (auto & : 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(AP2.first->getTypeForDecl(), 0), O2);
-O2.flush();
-
-if (S1 < S2)
-  return true;
-if (S1 != S2)
-  return false;
+QualType(N.Base->getTypeForDecl(), 0), Stream);
+AddressPoints.push_back(std::move(N));
+  }
 
-return AP1.second < AP2.second;
-  });
+  // Sort the address points for determinism.
+  llvm::sort(AddressPoints);
 
   ArrayRef 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
@@ -1357,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);
 }
   }

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Fangrui Song via cfe-commits]

https://github.com/MaskRay approved this pull request.


https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Reid Kleckner via cfe-commits]

https://github.com/rnk approved this pull request.


https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC][Clang][CodeGen] Improve performance for vtable metadata generation (PR #67066)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov edited 
https://github.com/llvm/llvm-project/pull/67066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits