[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno commandeered this revision.
riccibruno edited reviewers, added: Anastasia; removed: riccibruno.
riccibruno added a comment.

Closing this since the issue was fixed in r358674.


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Abandon this?
Sorry for this fruitless effort.


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

What about something like D60835  ?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60778#1470176 , @lebedev.ri wrote:

> In D60778#1470152 , @Anastasia wrote:
>
> > 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.
> >
> >
> > Ok I could do that, but I guess it can then fail on the commits that didn't 
> > actually trigger the issue. Would it not be a problem?
>
>
> Why would it fail if this fixes the issue?


But once something is changed and it's no longer deterministic, we might not be 
able to detect this necessarily on the offending commit with such test?

> Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60778#1470188 , @riccibruno wrote:

> Maybe, but is there an equivalent to `getTypeID` for declarations ?


`getDeclID` is used in the first patch...

although now I am thinking may be original patch is a better approach?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Maybe, but is there an equivalent to `getTypeID` for declarations ?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D60778#1470181 , @riccibruno wrote:

> Mmm. I hope I am not missing something obvious, but how is this actually 
> fixing the issue ? I don't see why the order between the `Type *` and between 
> the `Decl *` should be deterministic (I think they will be when they are part 
> of the same slab in the allocator, but I don't see why the slab ordering 
> should be stable).


Oh. Duh.
Can a custom comparator be provided for these `std::map`, equivalent to the one 
in https://reviews.llvm.org/D60379 ?
Something like `getTypeID(LHS.first->getCanonicalTypeInternal()) < 
getTypeID(RHS.first->getCanonicalTypeInternal())` ?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Mmm. I hope I am not missing something obvious, but how is this actually fixing 
the issue ? I don't see why the order between the `Type *` and between the 
`Decl *` should be deterministic (I think they will be when they are part of 
the same slab in the allocator, but I don't see why the slab ordering should be 
stable).


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D60778#1470152 , @Anastasia wrote:

> 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.
>
>
> Ok I could do that, but I guess it can then fail on the commits that didn't 
> actually trigger the issue. Would it not be a problem?


Why would it fail if this fixes the issue?
Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


Ok I could do that, but I guess it can then fail on the commits that didn't 
actually trigger the issue. Would it not be a problem?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: lebedev.ri, sylvestre.ledru, riccibruno.
Herald added subscribers: ebevhan, JDevlieghere, yaxunl.

As discussed in https://reviews.llvm.org/D60379 let's use std::map to store 
OpenCL extensions data structure.

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.


https://reviews.llvm.org/D60778

Files:
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTReader.h


Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -879,10 +879,10 @@
   OpenCLOptions OpenCLExtensions;
 
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap> OpenCLTypeExtMap;
+  std::map> OpenCLTypeExtMap;
 
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap> OpenCLDeclExtMap;
+  std::map> OpenCLDeclExtMap;
 
   /// A list of the namespaces we've seen.
   SmallVector KnownNamespaces;
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -8730,9 +8730,9 @@
 private:
   std::string CurrOpenCLExtension;
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap> OpenCLTypeExtMap;
+  std::map> OpenCLTypeExtMap;
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap> OpenCLDeclExtMap;
+  std::map> OpenCLDeclExtMap;
 public:
   llvm::StringRef getCurrentOpenCLExtension() const {
 return CurrOpenCLExtension;


Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -879,10 +879,10 @@
   OpenCLOptions OpenCLExtensions;
 
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap> OpenCLTypeExtMap;
+  std::map> OpenCLTypeExtMap;
 
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap> OpenCLDeclExtMap;
+  std::map> OpenCLDeclExtMap;
 
   /// A list of the namespaces we've seen.
   SmallVector KnownNamespaces;
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -8730,9 +8730,9 @@
 private:
   std::string CurrOpenCLExtension;
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap> OpenCLTypeExtMap;
+  std::map> OpenCLTypeExtMap;
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap> OpenCLDeclExtMap;
+  std::map> OpenCLDeclExtMap;
 public:
   llvm::StringRef getCurrentOpenCLExtension() const {
 return CurrOpenCLExtension;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits