[all-commits] [llvm/llvm-project] b1c2f1: [BasicAA] Move assumption tracking into AAQI

[Nikita Popov via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: b1c2f1282a237e9bc60f1b0020bc7535ca019739
      https://github.com/llvm/llvm-project/commit/b1c2f1282a237e9bc60f1b0020bc7535ca019739
  Author: Nikita Popov <nikita.ppv at gmail.com>
  Date:   2021-01-17 (Sun, 17 Jan 2021)

  Changed paths:
    M llvm/include/llvm/Analysis/AliasAnalysis.h
    M llvm/include/llvm/Analysis/BasicAliasAnalysis.h
    M llvm/lib/Analysis/BasicAliasAnalysis.cpp
    A llvm/test/Transforms/MemCpyOpt/aa-recursion-assertion-failure.ll

  Log Message:
  -----------
  [BasicAA] Move assumption tracking into AAQI

D91936 placed the tracking for the assumptions into BasicAA.
However, when recursing over phis, we may use fresh AAQI instances.
In this case AssumptionBasedResults from an inner AAQI can reesult
in a removal of an element from the outer AAQI.

To avoid this, move the tracking into AAQI. This generally makes
more sense, as the NoAlias assumptions themselves are also stored
in AAQI.

The test case only produces an assertion failure with D90094
reapplied. I think the issue exists independently of that change
as well, but I wasn't able to come up with a reproducer.


  Commit: 0b84afa5fcb41429004db72a0588656a8d76bf48
      https://github.com/llvm/llvm-project/commit/0b84afa5fcb41429004db72a0588656a8d76bf48
  Author: Nikita Popov <nikita.ppv at gmail.com>
  Date:   2021-01-17 (Sun, 17 Jan 2021)

  Changed paths:
    M llvm/include/llvm/Analysis/BasicAliasAnalysis.h
    M llvm/lib/Analysis/BasicAliasAnalysis.cpp
    M llvm/lib/Analysis/GlobalsModRef.cpp

  Log Message:
  -----------
  Reapply [BasicAA] Handle recursive queries more efficiently

There are no changes relative to the original commit. However, an issue
this exposed in BasicAA assumption tracking has been fixed in the
previous commit.

-----

An alias query currently works out roughly like this:

 * Look up location pair in cache.
 * Perform BasicAA logic (including cache lookup and insertion...)
 * Perform a recursive query using BestAAResults.
   * Look up location pair in cache (and thus do not recurse into BasicAA)
   * Query all the other AA providers.
 * Query all the other AA providers.

This is a lot of unnecessary work, all ultimately caused by the
BestAAResults query at the end of aliasCheck(). The reason we perform
it, is that aliasCheck() is getting called recursively, and we of
course want those recursive queries to also make use of other AA
providers, not just BasicAA. We can solve this by making the recursive
queries directly use BestAAResults (which will check both BasicAA
and other providers), rather than recursing into aliasCheck().

There are some tradeoffs:

 * We can no longer pass through the precomputed underlying object
   to aliasCheck(). This is not a major concern, because nowadays
   getUnderlyingObject() is quite cheap.
 * Results from other AA providers are no longer cached inside
   BasicAA. The way this worked was already a bit iffy, in that a
   result could be cached, but if it was MayAlias, we'd still end
   up re-querying other providers anyway. If we want to cache
   non-BasicAA results, we should do that in a more principled manner.

In any case, despite those tradeoffs, this works out to be a decent
compile-time improvment. I think it also simplifies the mental model
of how BasicAA works. It took me quite a while to fully understand
how these things interact.

Differential Revision: https://reviews.llvm.org/D90094


Compare: https://github.com/llvm/llvm-project/compare/3809f4ebabde...0b84afa5fcb4