[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358674: [Serialization] Stable serialization order for 
OpenCLTypeExtMap and… (authored by brunoricci, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60835?vs=195627=195744#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60835/new/

https://reviews.llvm.org/D60835

Files:
  cfe/trunk/lib/Serialization/ASTWriter.cpp


Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -4278,14 +4278,32 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLTypeExtMap by TypeIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLTypeExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLTypeExtMap;
+  StableOpenCLTypeExtMap.reserve(OpenCLTypeExtMap.size());
+
+  for (const auto  : OpenCLTypeExtMap)
+StableOpenCLTypeExtMap.emplace_back(
+getTypeID(I.first->getCanonicalTypeInternal()), );
+
+  auto CompareByTypeID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLTypeExtMap, CompareByTypeID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
-Record.push_back(I.second.size());
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLTypeExtMap) {
+Record.push_back(E.first); // TypeID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_TYPES, Record);
 }
 
@@ -4293,13 +4311,31 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLDeclExtMap by DeclIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLDeclExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLDeclExtMap;
+  StableOpenCLDeclExtMap.reserve(OpenCLDeclExtMap.size());
+
+  for (const auto  : OpenCLDeclExtMap)
+StableOpenCLDeclExtMap.emplace_back(getDeclID(I.first), );
+
+  auto CompareByDeclID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLDeclExtMap, CompareByDeclID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
-Record.push_back(static_cast(I.second.size()));
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLDeclExtMap) {
+Record.push_back(E.first); // DeclID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_DECLS, Record);
 }
 


Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -4278,14 +4278,32 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLTypeExtMap by TypeIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLTypeExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLTypeExtMap;
+  StableOpenCLTypeExtMap.reserve(OpenCLTypeExtMap.size());
+
+  for (const auto  : OpenCLTypeExtMap)
+StableOpenCLTypeExtMap.emplace_back(
+getTypeID(I.first->getCanonicalTypeInternal()), );
+
+  auto CompareByTypeID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLTypeExtMap, CompareByTypeID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
-Record.push_back(I.second.size());
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLTypeExtMap) {
+Record.push_back(E.first); // TypeID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_TYPES, Record);
 }
 
@@ -4293,13 +4311,31 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLDeclExtMap by DeclIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLDeclExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLDeclExtMap;
+  

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Anastasia Stulova via Phabricator via cfe-commits]

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

>> We could still go for something like the following but accept that some a 
>> random failure might happen not necessarily on a commit that introduces it?
> 
> I am not sure that this is needed. Non-deterministic tests are really 
> annoying.

Yes. I think this might be a wider topic to address if we happen to have more 
issues of this kind.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60835/new/

https://reviews.llvm.org/D60835



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

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D60835#1471498 , @Anastasia wrote:

> In D60835#1470805 , @riccibruno 
> wrote:
>
> > By the way, I am wondering about how much this is tested. I did look 
> > quickly in `test/PCH` and it appears that there are only 3 (short) tests : 
> > `ocl_types.cl`, `opencl-extensions.cl` and `no-validate-pch`.
>
>
> This is tested in  test/SemaOpenCL/extension-begin.cl: 
> https://reviews.llvm.org/D53200


Ah I see, I did indeed miss that.

> The ordering in PCH isn't tested anywhere however, but I am not clear how to 
> check for nondeterminism properly. Because the fact that the same output is 
> generated N times won't guarantee that it's the same N+1 times. Especially I 
> do believe it's likely on the same revision the same output to be produced 
> but it's less likely across revisions. :(
> 
> We could still go for something like the following but accept that some a 
> random failure might happen not necessarily on a commit that introduces it?

I am not sure that this is needed. Non-deterministic tests are really annoying.

> In D60778#1468825 , @lebedev.ri 
> wrote:
> 
>> > Not sure how to test this though? I guess we can trust that std::map is 
>> > always sorted just as it says in its description.
>>
>> You could add a test that contains several entries in `OpenCLTypeExtMap` and 
>> several entries in `OpenCLDeclExtMap`.
>>  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that PCH. 
>> It should dump it the bitcode in textual dump.
>>  And then simply apply FileCheck to that textual dump, with CHECK lines 
>> requiring the specific order of these entries.
>>  If the order is not deterministic in PCH, then that test would (should!) 
>> fail sporadically.
>>  At least that is my guess.




Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60835/new/

https://reviews.llvm.org/D60835



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

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60835#1470805 , @riccibruno wrote:

> By the way, I am wondering about how much this is tested. I did look quickly 
> in `test/PCH` and it appears that there are only 3 (short) tests : 
> `ocl_types.cl`, `opencl-extensions.cl` and `no-validate-pch`.


This is tested in  test/SemaOpenCL/extension-begin.cl: 
https://reviews.llvm.org/D53200

The ordering in PCH isn't tested anywhere however, but I am not clear how to 
check for nondeterminism properly. Because the fact that the same output is 
generated N times won't guarantee that it's the same N+1 times. Especially I do 
believe it's likely on the same revision the same output to be produced but 
it's less likely across revisions. :(

We could still go for something like the following but accept that some a 
random failure might happen not necessarily on a commit that introduces it?

In D60778#1468825 , @lebedev.ri wrote:

> > Not sure how to test this though? I guess we can trust that std::map is 
> > always sorted just as it says in its description.
>
> You could add a test that contains several entries in `OpenCLTypeExtMap` and 
> several entries in `OpenCLDeclExtMap`.
>  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that PCH. It 
> should dump it the bitcode in textual dump.
>  And then simply apply FileCheck to that textual dump, with CHECK lines 
> requiring the specific order of these entries.
>  If the order is not deterministic in PCH, then that test would (should!) 
> fail sporadically.
>  At least that is my guess.



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60835/new/

https://reviews.llvm.org/D60835



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

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

By the way, I am wondering about how much this is tested. I did look quickly in 
`test/PCH` and it appears that there are only 3 (short) tests : `ocl_types.cl`, 
`opencl-extensions.cl` and `no-validate-pch`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60835/new/

https://reviews.llvm.org/D60835



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

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: Anastasia, lebedev.ri.
riccibruno added a project: clang.
Herald added subscribers: cfe-commits, mgrang, yaxunl.

Sort the elements of `Sema::OpenCLTypeExtMap` and `Sema::OpenCLDeclExtMap` by 
`TypeID`s and `DeclID`s to guarantee a stable serialization order.


Repository:
  rC Clang

https://reviews.llvm.org/D60835

Files:
  lib/Serialization/ASTWriter.cpp


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4278,14 +4278,32 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLTypeExtMap by TypeIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLTypeExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLTypeExtMap;
+  StableOpenCLTypeExtMap.reserve(OpenCLTypeExtMap.size());
+
+  for (const auto  : OpenCLTypeExtMap)
+StableOpenCLTypeExtMap.emplace_back(
+getTypeID(I.first->getCanonicalTypeInternal()), );
+
+  auto CompareByTypeID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLTypeExtMap, CompareByTypeID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
-Record.push_back(I.second.size());
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLTypeExtMap) {
+Record.push_back(E.first); // TypeID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_TYPES, Record);
 }
 
@@ -4293,13 +4311,31 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLDeclExtMap by DeclIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLDeclExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLDeclExtMap;
+  StableOpenCLDeclExtMap.reserve(OpenCLDeclExtMap.size());
+
+  for (const auto  : OpenCLDeclExtMap)
+StableOpenCLDeclExtMap.emplace_back(getDeclID(I.first), );
+
+  auto CompareByDeclID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLDeclExtMap, CompareByDeclID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
-Record.push_back(static_cast(I.second.size()));
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLDeclExtMap) {
+Record.push_back(E.first); // DeclID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_DECLS, Record);
 }
 


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4278,14 +4278,32 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLTypeExtMap by TypeIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLTypeExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLTypeExtMap;
+  StableOpenCLTypeExtMap.reserve(OpenCLTypeExtMap.size());
+
+  for (const auto  : OpenCLTypeExtMap)
+StableOpenCLTypeExtMap.emplace_back(
+getTypeID(I.first->getCanonicalTypeInternal()), );
+
+  auto CompareByTypeID = [](const ElementTy , const ElementTy ) -> bool {
+return E1.first < E2.first;
+  };
+  llvm::sort(StableOpenCLTypeExtMap, CompareByTypeID);
+
   RecordData Record;
-  for (const auto  : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
-Record.push_back(I.second.size());
-for (auto Ext : I.second)
+  for (const ElementTy  : StableOpenCLTypeExtMap) {
+Record.push_back(E.first); // TypeID
+const std::set *ExtSet = E.second;
+Record.push_back(static_cast(ExtSet->size()));
+for (const std::string  : *ExtSet)
   AddString(Ext, Record);
   }
+
   Stream.EmitRecord(OPENCL_EXTENSION_TYPES, Record);
 }
 
@@ -4293,13 +4311,31 @@
   if (!SemaRef.Context.getLangOpts().OpenCL)
 return;
 
+  // Sort the elements of the map OpenCLDeclExtMap by DeclIDs,
+  // without copying them.
+  const llvm::DenseMap>  =
+  SemaRef.OpenCLDeclExtMap;
+  using ElementTy = std::pair *>;
+  llvm::SmallVector StableOpenCLDeclExtMap;
+  StableOpenCLDeclExtMap.reserve(OpenCLDeclExtMap.size());
+
+  for (const auto  : OpenCLDeclExtMap)
+StableOpenCLDeclExtMap.emplace_back(getDeclID(I.first), );
+
+  auto CompareByDeclID =