[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 a subscriber: hfinkel.

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


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


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:603
@@ +602,3 @@
+      AliasResult AR =
+          FAR ? FAR->alias(MemoryLocation(*CI), MemoryLocation(Object))
+              : alias(MemoryLocation(*CI), MemoryLocation(Object));
----------------
What is this doing? (when would we not have an FAR?

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:777
@@ -816,4 +776,3 @@
+        // assert.
         if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
           return MayAlias;
----------------
Why is it not an assert? It just returns MayAlias otherwise regardless.

================
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) {
----------------
Same here.

================
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) {
----------------
Same here.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:3080
@@ -3078,2 +3079,3 @@
   AU.addPreserved<DominatorTreeWrapperPass>();
+  AU.addPreserved<GlobalsAAWrapperPass>();
 }
----------------
Why is this here specifically?


================
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",
----------------
Do you also need?

  INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)

since you added it as required above?


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:65
@@ -63,3 +64,3 @@
       AU.setPreservesCFG();
-      AU.addPreserved<AliasAnalysis>();
+      AU.addPreserved<GlobalsAAWrapperPass>();
     }
----------------
I don't understand why you've added this to some passes and not others.


================
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)
----------------
Same question here about GlobalsAAWrapperPass and the INITIALIZE_* macro.

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


http://reviews.llvm.org/D12080