[PATCH] D17329: [AA] Hoist the logic to reformulate various AA queries in terms of other parts of the AA interface out of the base class of every single AA result object.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Analysis/AliasAnalysis.cpp:139
@@ +138,3 @@
+  // aggregate set of AA results.
+  auto MRB = getModRefBehavior(CS);
+  if (MRB == FMRB_DoesNotAccessMemory)
----------------
Gerolf wrote:
> chandlerc wrote:
> > Gerolf wrote:
> > > This routine queries AA again but this time for behavior rather than ModRefInfo, but the result (no mod ref info) could be the same. Can this be combined in some meta routine so that no mod ref is spit out early? The AA loops can be very compile-time expensive so we should avoid/shorten them when possible.
> > This is interesting, but seems really orthogonal to this change. I don't want to try to fix everything in one go, I'm trying to fix a specific problem with the existing implementation.
> My concern here is compile-time. The AA loops are compile-time intense and this code could make it worse. For a change like this it would be great if you could collect compile-time data before you commit. 
I'm confused...

As far as I can tell, this change *cannot* make the compile time worse. Previously, this exact code existed in *every* base class for an AAResults, and would have been reached by the loop above for each one of them. That means it would have been run at least once (via BasicAA) and in many cases N times for the number of AAs active. In common Clang compiles that would be at least 4 times. So we're running the same code 1/4th as often with my change.

Unless I've missed something?

================
Comment at: lib/Analysis/AliasAnalysis.cpp:149
@@ +148,3 @@
+    ModRefInfo AllArgsMask = MRI_NoModRef;
+    if (doesAccessArgPointees(MRB)) {
+      for (auto AI = CS.arg_begin(), AE = CS.arg_end(); AI != AE; ++AI) {
----------------
Gerolf wrote:
> Gerolf wrote:
> > chandlerc wrote:
> > > Gerolf wrote:
> > > > Could you refactor into a function?
> > > This is directly moving code from one place to another here. I don't really want to refactor all of it to look different at the same time. This applies to most of the remaining comments as well -- all of this code existed pretty much exactly as-is before. But if there is a desire to refactor this, I think that should be a separate patch.
> > My concern is compile-time. The AA loops are compile-time intense and I suspect this code makes it worse. Do your data show that is not the case?
> Makes sense. Also digging a bit more into the code this is not as straightforward as I thought it was.
See above for why this too cannot (AFAICT) be making the compile time worse.

================
Comment at: lib/Analysis/AliasAnalysis.cpp:171
@@ +170,3 @@
+  // modify the memory location.
+  if ((Result & MRI_Mod) &&
+      pointsToConstantMemory(Loc, /*OrLocal*/ false))
----------------
Gerolf wrote:
> chandlerc wrote:
> > Gerolf wrote:
> > > Why could Mod be set in this case? Wouldn't that be a bug?
> > If this is a bug, it is a bug in existing code, so you should be able to find a test case and fix it separately from this patch?
> Why does the code check for this condition rather asserting? I'm wondering how the code above could diagnose a Mod for a constant location.
I understand, but this code is *not new* in this patch. I am moving it from one location to another, I just had to change the name of a variable.


http://reviews.llvm.org/D17329