rtrieu added a comment.
This patch will be landing in small chunks to hopefully avoid the large reverts.
Repository:
rL LLVM
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mai
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293585: Add better ODR checking for modules. (authored by
rtrieu).
Changed prior to commit:
https://reviews.llvm.org/D21675?vs=86142&id=86376#toc
Repository:
rL LLVM
https://reviews.llvm.org/D21675
rtrieu updated this revision to Diff 86142.
rtrieu added a comment.
Changes made to the ODR hash algorithm:
Separated Decl and Type pointers into their own DenseMap's.
Removed the queue of pointers to process at the end. Instead, process pointers
at first use.
Save Boolean values and add them t
rsmith added a comment.
In https://reviews.llvm.org/D21675#654659, @teemperor wrote:
> Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection
> use the same backend.
This code is *already* reusing the Stmt::Profile code for hashing of
statements. Why was a different mech
rtrieu added a comment.
In https://reviews.llvm.org/D21675#654659, @teemperor wrote:
> I feel like we have a really similar use case in the
> Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried
> substituting the call to the statement hashing with a call to the
> CloneDete
teemperor added a comment.
I feel like we have a really similar use case in the
Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried
substituting the call to the statement hashing with a call to the
CloneDetection API and it seems that most tests pass and the remaining fails
rtrieu added a comment.
After changing the ODRHash class a bit, the new performance numbers are 3% in
debug and 1-1.5% in release builds.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/
rtrieu marked an inline comment as done.
rtrieu added a comment.
From testing, there is no difference when compiling with pre-compiled headers.
However, when building the headers, there is a 3-4% impact on compile time.
https://reviews.llvm.org/D21675
___
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks, looks good assuming your performance testing doesn't uncover anything.
Comment at: lib/AST/ODRHash.cpp:319-321
+if (!D) return;
+if (D->isImplicit())
+ r
rtrieu marked 5 inline comments as done.
rtrieu added a comment.
Comments have been addressed. I will be testing it for performance impact next.
Comment at: include/clang/AST/ODRHash.h:54
+ // in Pointers.
+ size_type NextFreeIndex;
+
rsmith wrote:
> Is this
rtrieu updated this revision to Diff 84340.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/ODRHash.h
include/clang/AST/Stmt.h
include/clang/Basic/DiagnosticSerializationKinds.td
lib/AST/CMakeLists.txt
lib/AST/DeclCXX.cpp
lib/AST/ODRHash.cpp
li
rsmith added a comment.
Thanks! I assume there's still no measurable performance impact?
Comment at: include/clang/AST/ODRHash.h:54
+ // in Pointers.
+ size_type NextFreeIndex;
+
Is this always the same as `Pointers.size()`?
Comment at: lib
rtrieu updated this revision to Diff 82702.
rtrieu added a comment.
Herald added a subscriber: mgorny.
This is a redesign of the ODR checker which was discovered to have several
shortcomings when run over test cases. The old version mainly used depth-first
processing of AST nodes to gather the
rtrieu added a comment.
In https://reviews.llvm.org/D21675#560297, @rsmith wrote:
> There are a bunch of cases here that do this:
>
> if (auto t = getThing())
> ID.addThing(t);
> if (auto t = getOtherThing())
> ID.addThing(t);
>
>
> That will result in hash collisions between objects
rtrieu updated this revision to Diff 75970.
rtrieu marked 2 inline comments as done.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclBase.h
include/clang/AST/DeclCXX.h
include/clang/AST/Stmt.h
include/clang/AST/TemplateBase.h
include/clang/AST/Type.h
include/clang/Basic/D
rsmith added a comment.
There are a bunch of cases here that do this:
if (auto t = getThing())
ID.addThing(t);
if (auto t = getOtherThing())
ID.addThing(t);
That will result in hash collisions between objects with thing and objects with
otherthing (for instance, `struct A { int n :
rtrieu marked 2 inline comments as done.
rtrieu added inline comments.
> dblaikie wrote in DeclBase.cpp:1810-1812
> Inconsistent {} on single line block (in VisitEnumConstantDecl above {} are
> not used on a single line block) - usually drop the {} on single line blocks.
>
> (several other inst
rtrieu updated this revision to Diff 73388.
rtrieu added a comment.
Add a more detailed error message to let users know where the two records
differ. This replaces the generic error which only stated that two definitions
are different without any details.
https://reviews.llvm.org/D21675
File
dblaikie added inline comments.
Comment at: lib/AST/DeclBase.cpp:1810-1812
@@ +1809,5 @@
+ void VisitNamedDecl(const NamedDecl *D) {
+if (IdentifierInfo *II = D->getIdentifier()) {
+ ID.AddString(II->getName());
+}
+Inherited::VisitNamedDecl(D);
rtrieu updated this revision to Diff 68278.
rtrieu added a comment.
Add function void ODRHash(llvm::FoldingSetNodeID &ID) to several classes for
computing the hash. Decl, Stmt, TemplateArgument, Type and QualType now have
this function, and can call among each others' functions.
https://revie
rtrieu created this revision.
rtrieu added a subscriber: cfe-commits.
Early prototype of an improved ODR checker for Clang and modules. The current
ODR checking of classes is limited to a small number of attributes such as
number of methods and base classes. This still allows a number of ODR
21 matches
Mail list logo
