[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis closed this revision.
thakis marked an inline comment as done.
thakis added a comment.

r326960, thanks!


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1207
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(VBase).getQuantity();
 

The `getVBaseClassOffset` call turns around and does the same hashtable lookup 
we just did. Let's keep the iterator around and say 
`I->second.VBaseOffset.getQuantity()`.


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis marked an inline comment as done.
thakis added inline comments.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1206
+
+  for (const VBase  : VBases) {
+if (!V.second.hasVtorDisp())

rnk wrote:
> I think we can avoid the sort altogether if we iterate `RD->vbases()`, which 
> should already be in layout order, and then do lookup into `VBaseMap`.
That's way nicer, thanks. Done.


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis updated this revision to Diff 137470.
thakis added a comment.

rnk comment


https://reviews.llvm.org/D44223

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,15 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  for (const CXXBaseSpecifier  : RD->vbases()) {
+const CXXRecordDecl *VBase = S.getType()->getAsCXXRecordDecl();
+if (!VBaseMap.find(VBase)->second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, VBase);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(VBase).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,15 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  for (const CXXBaseSpecifier  : RD->vbases()) {
+const CXXRecordDecl *VBase = S.getType()->getAsCXXRecordDecl();
+if (!VBaseMap.find(VBase)->second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, VBase);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(VBase).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Thanks for investigating!




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1206
+
+  for (const VBase  : VBases) {
+if (!V.second.hasVtorDisp())

I think we can avoid the sort altogether if we iterate `RD->vbases()`, which 
should already be in layout order, and then do lookup into `VBaseMap`.


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis updated this revision to Diff 137463.
thakis added a comment.
Herald added a subscriber: mgrang.

sort unstably


https://reviews.llvm.org/D44223

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,21 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  // Emit vtordisps in vbase offset order, to have deterministic output.
+  typedef VBOffsets::value_type VBase;
+  SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+  std::sort(VBases.begin(), VBases.end(), [](const VBase , const VBase ) {
+return a.second.VBaseOffset < b.second.VBaseOffset;
+  });
+
+  for (const VBase  : VBases) {
+if (!V.second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, V.first);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(V.first).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,21 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  // Emit vtordisps in vbase offset order, to have deterministic output.
+  typedef VBOffsets::value_type VBase;
+  SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+  std::sort(VBases.begin(), VBases.end(), [](const VBase , const VBase ) {
+return a.second.VBaseOffset < b.second.VBaseOffset;
+  });
+
+  for (const VBase  : VBases) {
+if (!V.second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, V.first);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(V.first).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1202
+  SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+  std::stable_sort(VBases.begin(), VBases.end(),
+   [](const VBaseEntry , const VBaseEntry ) {

Using stable_sort() here, as opposed to sort(), doesn't do anything useful 
here; VBaseMap is a DenseMap with a pointer key, so the input is in random 
order anyway.


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1207
+
+  for (const VBaseEntry E : VBases) {
+if (!E.second.hasVtorDisp())

(I added the missing `&` here locally.)


https://reviews.llvm.org/D44223



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

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: rnk.

No effective behavior change, just for cleanliness.

Fixes PR36159.


https://reviews.llvm.org/D44223

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,22 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  // Emit vtordisps in vbase offset order, to have deterministic output.
+  typedef VBOffsets::value_type VBaseEntry;
+  SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+  std::stable_sort(VBases.begin(), VBases.end(),
+   [](const VBaseEntry , const VBaseEntry ) {
+ return a.second.VBaseOffset < b.second.VBaseOffset;
+   });
+
+  for (const VBaseEntry E : VBases) {
+if (!E.second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, E.first);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(E.first).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(


Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1196,15 +1196,22 @@
   unsigned AS = getThisAddress(CGF).getAddressSpace();
   llvm::Value *Int8This = nullptr;  // Initialize lazily.
 
-  for (VBOffsets::const_iterator I = VBaseMap.begin(), E = VBaseMap.end();
-I != E; ++I) {
-if (!I->second.hasVtorDisp())
+  // Emit vtordisps in vbase offset order, to have deterministic output.
+  typedef VBOffsets::value_type VBaseEntry;
+  SmallVector VBases(VBaseMap.begin(), VBaseMap.end());
+  std::stable_sort(VBases.begin(), VBases.end(),
+   [](const VBaseEntry , const VBaseEntry ) {
+ return a.second.VBaseOffset < b.second.VBaseOffset;
+   });
+
+  for (const VBaseEntry E : VBases) {
+if (!E.second.hasVtorDisp())
   continue;
 
 llvm::Value *VBaseOffset =
-GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, I->first);
+GetVirtualBaseClassOffset(CGF, getThisAddress(CGF), RD, E.first);
 uint64_t ConstantVBaseOffset =
-Layout.getVBaseClassOffset(I->first).getQuantity();
+Layout.getVBaseClassOffset(E.first).getQuantity();
 
 // vtorDisp_for_vbase = vbptr[vbase_idx] - offsetof(RD, vbase).
 llvm::Value *VtorDispValue = Builder.CreateSub(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits