[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: include/llvm/Analysis/AliasAnalysis.h:163
@@ +162,3 @@
+  // Make these results default constructable and movable. We have to spell
+  // these out because MSVC won't synthesize them.
+  FunctionAAResults() {}
----------------
chandlerc wrote:
> hfinkel wrote:
> > Which version of MSVC? I'd like that recorded if possible so that when we eventually drop support for that version we can remove the workarounds.
> > 
> We don't do this elsewhere. This comment is essentially verbatim from everywhere else we've commented this. We can go and update all of them, but please not in this patch.
> We don't do this elsewhere.

Well, that's a mistake, but I'm fine with consistency. Let's update the comments in a separate change.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:402
@@ +401,3 @@
+  // Populate the results with the currently available AAs.
+  if (auto *WrapperPass = getAnalysisIfAvailable<ScopedNoAliasAAWrapperPass>())
+    AAResults->addAAResult(WrapperPass->getResult());
----------------
chandlerc wrote:
> hfinkel wrote:
> > This design means that we cannot add new AA passes, nor change their order via the pass-manager interface, because it is really hard-coded here for a closed universe of AA passes. Is this really necessary? I'd prefer we have some kind of registration interface.
> > 
> First off, I feel like you're missing a key point in the design.
> 
> The *infrastructure* here doesn't hardcode anything. It is only the "factory" that we use with the legacy pass manager that hard codes a list of the AAs. The reason is because it is expedient. I'm really uninterested in building complex registration facilities for the legacy pass manager, especially as it is *exactly* such a registration facility (and all of its bugs, limitations and problems) that I'm removing with this patch. =/
> 
> I've also arranged for it to be OK to add experimental, disabled, and otherwise unused AAs here so we can still experiment and develop things in-tree.
> 
> 
> The new pass manager will be able to use all this infrastructure to build a custom set of alias analyses easily. We will still probably "hard code" a list *somewhere*, but it will be much higher up the stack in the Passes library. And there, rather than actually hard coding the list, my plan was to parse the list out of the command line.
> 
> 
> The core idea of having a single place that *specifically* spells out what alias analyses are used and exactly what order was actually a design point that I discussed with several folks, yourself included, and I thought was an explicit desire. Personally, I find using a registration system to establish the layering of alias analyses extremely brittle and confusing.
Okay; this is fine. But, please add a comment to this function (and something in getAnalysisUsage) explaining that this fixed list/ordering is relevant only for the legacy pass manager.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:603
@@ +602,3 @@
+      AliasResult AR =
+          FAR ? FAR->alias(MemoryLocation(*CI), MemoryLocation(Object))
+              : alias(MemoryLocation(*CI), MemoryLocation(Object));
----------------
chandlerc wrote:
> hfinkel wrote:
> > What is this doing? (when would we not have an FAR?
> You can create a BasicAAResults object without using the FunctionAAResults aggregation layer. I didn't want to bake the cycle into the entire implementation. I can go back and do that if you think it's the right thing, but it makes this not *really* an analysis pass any longer, which I find very unfortunate.
Okay, please add a comment explaining this. The key question is this: If I were looking at this code in order to write new code in BasicAAResult, or in some other *AAResult class, would I need to do this?


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:777
@@ -816,4 +776,3 @@
+        // assert.
         if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
           return MayAlias;
----------------
chandlerc wrote:
> hfinkel wrote:
> > Why is it not an assert? It just returns MayAlias otherwise regardless.
> I really didn't want to be adding asserts that might crash in the wild and might make a well over 100 file patch get reverted. ;]
Okay ;)

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:3106
@@ -3103,3 +3105,3 @@
 INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
-INITIALIZE_AG_DEPENDENCY(AliasAnalysis)
+INITIALIZE_PASS_DEPENDENCY(FunctionAAResultsWrapperPass)
 INITIALIZE_PASS_END(InstructionCombiningPass, "instcombine",
----------------
chandlerc wrote:
> hfinkel wrote:
> > Do you also need?
> > 
> >   INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
> > 
> > since you added it as required above?
> > 
> Yes, good catch. I've added it.
Please check them all; I stopped commenting on them at some point.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:127
@@ -123,2 +126,3 @@
       AU.addPreserved<ScalarEvolutionWrapperPass>();
+      AU.addPreserved<SCEVAAWrapperPass>();
       AU.addRequired<TargetLibraryInfoWrapperPass>();
----------------
> Preserving SCEVAA above is actually a bug, and I've removed it.

Why was it a bug? We preserve SCEV here, and we also used to preserve the AA group.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:71
@@ -68,3 +70,3 @@
 INITIALIZE_PASS_DEPENDENCY(LoopSimplify)
 INITIALIZE_PASS_DEPENDENCY(LCSSA)
 INITIALIZE_PASS_END(LoopDeletion, "loop-deletion",
----------------
Do you need an INITIALIZE_ for SCEVAA here?


http://reviews.llvm.org/D12080