martong abandoned this revision.
martong added a comment.
@a_sidorin Alexei,
With Balazs we had a long discussion and I think we reached a consensus and
common understanding. Now I try to summarize it.
It turned out that this patch essentially renders the `NonEquivalentDecls`
cache to be completely disabled, i.e. we never have a cache hit. (I tried an
example test run with an assertion enabled for the cache hit.)
This is because in `Finish()`, when we find the first non-equivalent pair then
we immediately break out from the while loop which pops out elements from the
queue. This means we do not consider any elements left in the queue after this,
so the "local" cache is never going to be used again.
This, however, explains why did we have better results with this patch: the
cache was disabled, so those faulty entries which were cached (see Balazs's
example above) could not cause a faulty behavior.
Secondly, I tried to fabricate a test case where the change of the redecl chain
could render previously non-equivalent pairs to be equivalent. I could not
create such. I also executed an experiment on llvm/master to see in case of a
cache hit whether we would get a different result without the cache:
// Check whether we already know that these two declarations are not
// structurally equivalent.
if (Context.NonEquivalentDecls.count(
- std::make_pair(D1->getCanonicalDecl(), D2->getCanonicalDecl())))
+ std::make_pair(D1->getCanonicalDecl(), D2->getCanonicalDecl()))) {
+ llvm::errs() << "Cache hit\n";
+ llvm::DenseSet<std::pair<Decl *, Decl *>> LocalNeqDecls;
+ StructuralEquivalenceContext LocalCtx(Context.FromCtx, Context.ToCtx,
+ LocalNeqDecls, Context.EqKind, false,
+ false, false);
+ assert(!LocalCtx.IsEquivalent(D1, D2));
return false;
+ }
The added assertion did not fire. (I executed it on Xerces for a ~100 TUs). So,
it is highly probable that if some pairs were ever non-equivalent they will
stay non-equivalent.
This is not true, however, the other way around. Equivalent decls may become
non-equivalent (e.g. a fwd decl is being completed).
To solve the issue that is found by Balazs, I suggest to move on with
https://reviews.llvm.org/D66538 and abandon this patch.
An alternative solution would be to cache after the `Finish()` has returned,
but I prefer D66538 <https://reviews.llvm.org/D66538> because it makes the
whole algorithm simpler, plus we can get rid of the redecleration chain check.
By getting rid of that, I believe we can have a more robust import mechanism,
which is more resistant to lookup errors. ATM, if we have a lookup error then
we end up having two identical declarations in different redecl chains, which
may initiate a NameConflict error avalanche. With D66538
<https://reviews.llvm.org/D66538> we can avoid that.
As a next step, probably we should remove the cache because it complicates the
code and the added performance value is not recognizable. If you'd like I can
do measurements to investigate the performance difference. However, on our
internal fork we disabled the cache entirely for quite a long time (this patch
is originated from there) and we did not experience any noticeable performance
degradation during the analysis.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62131/new/
https://reviews.llvm.org/D62131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
