[PATCH] D110368: [AA] Move earliest escape tracking from DSE to AA

Sat Sep 25 12:18:46 PDT 2021

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

In D110368#3021556 <https://reviews.llvm.org/D110368#3021556>, @nikic wrote:

> In D110368#3020035 <https://reviews.llvm.org/D110368#3020035>, @fhahn wrote:
>
>>> This combines the "earliest escape" analysis with the full power of BasicAA:
>>
>> Are there any test cases that show the benefit of that, other than the improvements for the load-load case discussed in D109844 <https://reviews.llvm.org/D109844> ?
>
> See the last three tests in captures-before-load.ll. Only the last one truly requires BasicAA integration though.

Great thanks!

> Ultimately my main interest here is to integrate this functionality in a way that does not require reimplementing parts of AA in DSE. Better analysis quality is just a nice side effect.

Agreed, just wanted to double check w.r.t. the description.

LGTM, I am just wondering if the change in `isEscapeSource` is required or can be split off?

================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:383
+/// Virtual base class for providers of capture information.
+class CaptureInfo {
+public:
----------------
nit: could just be a struct?

================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:407
+  const LoopInfo &LI;
+  DenseMap<const Value *, Instruction *> EarliestEscapes;
+  DenseMap<Instruction *, TinyPtrVector<const Value *>> Inst2Obj;
----------------
I should have added doc-comments in the original DSE version but unfortunately did not. Might be good to add some here.

================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:123

-  if (isa<Argument>(V))
-    return true;
----------------
this seems unrelated or could at least be split off?

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

https://reviews.llvm.org/D110368