[PATCH] D12080: [PM/AA] Rebuild LLVM's alias analysis infrastructure in a way compatible with the new pass manager, and no longer relying on analysis groups.

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:603
@@ +602,3 @@
+      AliasResult AR =
+          AAR ? AAR->alias(MemoryLocation(*CI), MemoryLocation(Object))
+              : alias(MemoryLocation(*CI), MemoryLocation(Object));
----------------
chandlerc wrote:
> hfinkel wrote:
> > That comment is good, but let me clarify something: Is this the general pattern necessary for recursion in AA? Specifically, if there is an aggregation object, then recurse through the aggregation object so the entire chain participates, otherwise, just recurse directly? If this is the general pattern, then we should add it as a utility function and not repeat it everywhere it might be necessary. In short, what to do when one AA can reduce an aliasing query to another one on some underlying pointers should be something that is clear within the interface.
> > 
> Any suggestions about *how* to factor this out? I tried to come up with a way, and it involved horrible templates and pointers-to-members or std::bind craziness. I'll try again myself...
Maybe just type out the functions? I've always found this pattern difficult to template (as you point out, you can do it with pointers-to-members, etc., but the number of tokens in the template glue relative to the underlying logic might be quite large).

My concern is not that that the if statement is a lot of code to copy-and-paste, but that because code might appear functional without it (by just hard coding a direct recursion within the pass), it could easily be forgotten unless it is somehow builtin to the interface that the existing implementations all use.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1237-1238
@@ -1279,4 +1236,4 @@
   AliasResult Result =
-      AliasAnalysis::alias(MemoryLocation(V1, V1Size, V1AAInfo),
-                           MemoryLocation(V2, V2Size, V2AAInfo));
+      AAResultBase::alias(MemoryLocation(V1, V1Size, V1AAInfo),
+                          MemoryLocation(V2, V2Size, V2AAInfo));
   return AliasCache[Locs] = Result;
----------------
chandlerc wrote:
> I really don't know... The old code had different fallback strategies and it wasn't always clear to me whether they would fully recurse or partially recurse, etc.
> 
> In particular, does this inf-loop? Anyways, I'll try to make this more rigorous. I need to find out if it would infloop and the old code was trying to delegate to the *other* AAs by doing this, and if so, have it return MayAlias.
As far as I can tell, in the old system, calling the base class here (AliasAnalysis::alias), would cause the query to continue up the chain with the new pointers (in this case, likely the same as the original pointers, except perhaps with casts stripped).

So maybe I'm confused about what this code does, but it looks like  AAResultBase::alias just returns MayAlias, and that does not match the old behavior.


http://reviews.llvm.org/D12080