[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.

[Chandler Carruth via llvm-commits]

chandlerc marked 2 inline comments as done.

================
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() {}
----------------
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.

================
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());
----------------
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.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:603
@@ +602,3 @@
+      AliasResult AR =
+          FAR ? FAR->alias(MemoryLocation(*CI), MemoryLocation(Object))
+              : alias(MemoryLocation(*CI), MemoryLocation(Object));
----------------
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.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:777
@@ -816,4 +776,3 @@
+        // assert.
         if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
           return MayAlias;
----------------
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. ;]

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:813
@@ -853,2 +812,3 @@
     // same result except when DecomposeGEPExpression has no DataLayout.
+    // FIXME: They always have a DataLayout so this should become an assert.
     if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
----------------
hfinkel wrote:
> Same here.
Same answer.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:863
@@ -903,2 +862,3 @@
     // same result except when DecomposeGEPExpression has no DataLayout.
+    // FIXME: They always have a DataLayout so this should become an assert.
     if (GEP1BasePtr != UnderlyingV1) {
----------------
hfinkel wrote:
> Same here.
And same answer.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:3080
@@ -3078,2 +3079,3 @@
   AU.addPreserved<DominatorTreeWrapperPass>();
+  AU.addPreserved<GlobalsAAWrapperPass>();
 }
----------------
hfinkel wrote:
> Why is this here specifically?
> 
I explained this in my summary: everywhere we previously were preserving the analysis group I have added preservation of globals-aa to preserve behavior prior to my patch.

================
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",
----------------
hfinkel wrote:
> Do you also need?
> 
>   INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
> 
> since you added it as required above?
> 
Yes, good catch. I've added it.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:65
@@ -63,3 +64,3 @@
       AU.setPreservesCFG();
-      AU.addPreserved<AliasAnalysis>();
+      AU.addPreserved<GlobalsAAWrapperPass>();
     }
----------------
hfinkel wrote:
> I don't understand why you've added this to some passes and not others.
> 
As I explained in the summary (i think?) I've added it everywhere that previously preserved AliasAnalysis.

Note that it *seems* to have been removed from some, but those are MachineFunctionPasses and I added the preservation to the common logic for all of those.

I've checked, and I *think* I got this right, but if there is an inconsistency there, let me know.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:742
@@ -740,3 +741,3 @@
 INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
-INITIALIZE_AG_DEPENDENCY(AliasAnalysis)
+INITIALIZE_PASS_DEPENDENCY(FunctionAAResultsWrapperPass)
 INITIALIZE_PASS_END(GVN, "gvn", "Global Value Numbering", false, false)
----------------
hfinkel wrote:
> Same question here about GlobalsAAWrapperPass and the INITIALIZE_* macro.
Done here as well.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:127
@@ -123,2 +126,3 @@
       AU.addPreserved<ScalarEvolutionWrapperPass>();
+      AU.addPreserved<SCEVAAWrapperPass>();
       AU.addRequired<TargetLibraryInfoWrapperPass>();
----------------
hfinkel wrote:
> I understand that SCEVAA needs to be added, because of the LPM invalidation restrictions, but why is GlobalsAAWrapperPass here and not in IndVarSimplify?
Because IndVarSimplify did not historically preserve AliasAnalysis and so preserving GlobalsAAWrapperPass would change behavior.

I think every function pass can preserve GlobalsAAWrapperPass after my changes to make it more conservative, but I've tried very hard to change is little functionality as conceivably possible with this patch.

Preserving SCEVAA above is actually a bug, and I've removed it.


http://reviews.llvm.org/D12080