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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc 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.
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315