[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:
> 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?


================
Comment at: lib/Analysis/MemorySSA.cpp:1164-1169
+  // We must also tell the walker about the new AAResults,
+  // because currently the ClobberWalker stores a reference to AA, and it's
+  // being populated with the AA set before calling buildMemorySSA.
+  // e.g. add a call to WalkerBase->updateAA(this->AA); after buildMemorySSA(),
+  // and add an updateAA(AA) in ClobberWalker, and store a pointer to AA
+  // instead.
----------------
chandlerc wrote:
> This seems a bit weird to me... I would hope that there is a better way to plumb the batched AA wrapper into these APIs, but maybe I don't understand why its difficult?
> 
> However, it does raise a specifically interesting point: the walkers should use the batched interface here, but likely *not* use the batched interface after MemorySSA has been built as at that point the IR is likely changing and the batched results are no longer valid.
> 
> This seems like an important distinction that should be made more explicit. Maybe the walkers that the rest of the code use should be distinct from those used to build MemorySSA initially?
That is exactly right. The batched results are no longer valid after MemorySSA is built, and should no longer be used.
There is shared code between build-time and after-build.
This code remains the same and it's stateless, other than storing a reference to AA, DT etc. So either the walker needs updated to the non-batched AA results or re-initialized, as you suggest.
Not a big deal as far as plumbing, just something that needs fixing with the current code.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315