[PATCH] D59315: [AliasAnalysis] Second prototype to cache BasicAA / anyAA state.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Analysis/MemorySSA.cpp:1158-1163
+  // Alternative to modifying AA in place: create new AAResults, destroy right
+  // after. With this alternative, the following APIs go away:
+  // AAResults::setAAToBatch(), AAResults::setAAToSingle(),
+  // AAQueryInfo::reset().
+  // However this alternative needs to add a way to copy all AAs, or use teh
+  // move constructor and re-move the AAs back after building MemorySSA.
----------------
chandlerc wrote:
> asbirlea wrote:
> > chandlerc wrote:
> > > I think using an object to model a batch of queries is going to be significantly cleaner long term. As mentioned in the discussion, in part this is because I think we'll want to have this as an actual analysis that simply is invalidated *much* more eagerly than the "normal" AA analyses (by any change to the IR at all).
> > > 
> > > To address the move issue -- my suggestion would be to have it *wrap* the AAs in place rather than moving them. I'm not seeing why this won't work? Maybe just need more explanation here.
> > For wrapping, do you have something like this in mind?:
> > ```
> > class BatchAAResults : public SomeNewNameAAResultsInterface { //new
> >      BatchAAResults(AAResults &aar) : AAR(aar), AAQI(false) {}
> >      alias(...) {
> >          AAR.alias(..., AAQI);
> >      }
> >      // wrap all APIs and pass local AAQI.
> >      AAResults &AAR;
> >      AAQueryInfo AAQI;
> > }
> > 
> > class AAResults : public SomeNewNameAAResultsInterface {
> >     .... // keep the same
> > } 
> > 
> > using AliasAnalysis = SomeNewNameAAResultsInterface;
> > // currently: using AliasAnalysis = AAResults;
> > ```
> > 
> > or 
> > 
> > ```
> > class AAResults {  //existing, updates below
> >      AAResults(AAResults *aar) : TLI(aar->TLI), AAR(aar), AAQI(false) {}
> >      alias(...) {
> >          if (Inner)
> >             return Inner->alias(..., AAQI);
> >          // same as before
> >      }
> >      // update all APIs to delegate to Inner (if it exists), with local AAQI.
> > 
> >      ....
> >      AAResults *Inner = nullptr; // add to existing AAResults
> >      AAQueryInfo AAQI;
> > }
> > ```
> > 
> > I played with the second option before sending this out, and it works fine.
> > The first option probably needs additional changes to replace uses of AAResults with new name assigned to AliasAnalysis.
> > Preferences?
> > 
> > 
> > Cases that may require more extensive code changes, when the batch AA becomes another pass, happen where the same code needs to use an AA pass at some point, then use another later. This is the case for the comment below.
> > Having the objects/passes have a common `SomeNewNameAAResultsInterface` (first option) or `AAResults` (second option) would enable swapping a new object in to build and the old back in after.
> > Does this make sense?
> The first, but without the `using AliasAnalysis = ...` change.
> 
> Essentially, it seems like the best result is that the code that is doing batched queries says so explicitly (by using the batched wrapper type to query).
> 
> Is the issue that MemorySSA (and/or the walker) don't do all of the queries directly, but instead delegate some (most?) of the queries to generic library code that expects a (non-batched) type today? How often does this happen?
MemorySSA is just keeping an `AliasAnalysis* AA;` and doing `AA->alias()` or `AA->getModRefInfo()` calls.

The change would be to take in argument `AliasAnalysis* AA;` (like it is already doing), but store it internally as an `SomeNewNameAAResultsInterface *InternalAA`.
Building code becomes:
```
{
BatchAAResults  BatchAA(AA);
InternalAA = &BatchAA;
buildMemorySSA();
InternalAA = AA;
// BatchAA dies now. Assume the walker gets the AA by querying InternalAA.
}
```

As long as `SomeNewNameAAResultsInterface` has the same APIs, all will be fine. The reason for going to the "top-level type" is that the same code is using different AAs based on when it's being called (at build time or after).

Is this what you had in mind? Or actually changing all callsites to alias/getModRef in MemorySSA to check if you're in build stage or not, and make the call on a different object?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315