[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

[Gabor Marton via Phabricator via cfe-commits]

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