[PATCH] D21921: [proof of concept] Port old PM CGSCC visitation logic to new PM

[Davide Italiano via llvm-commits]

davide added a comment.

First round of comments. I feel like this patch is important because it shows up the old semantic can be ported to the new PM with (relatively) little effort. I don't quite understand all the corner cases of the CGSCC pass manager but your patch looks to me kinda mechanical (that's actually great because it means the old code fits in the new model nicely). I'll probably comment further, and take another look. As Chandler spent a fair amount of time on the problem I'd really love to hear what's his feedback/what are his comments. While I think that eventually we're going to land his work probably I feel this is still a valuable incremental step (and NFC).

Thanks!


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:104
@@ +103,3 @@
+
+    // XXX: no existing CGSCC passes use this.
+    //bool Changed = doInitialization(CG);
----------------
just remove it ? =)

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:146
@@ -129,1 +145,3 @@
+    // done?
+    //Changed |= doFinalization(CG);
     return PA;
----------------
or maybe move this stuff to the destructor? I think Chandler mentioned that's the equivalent of doFinalization() in the new PM

================
Comment at: include/llvm/Analysis/CallGraphSCCPass.h:96
@@ +95,3 @@
+  // XXX: hack for CGSCC pass manager to pass stuff back up to the
+  // ModuleToPostOrderCGSCCPassAdaptor's main visitation loop.
+  // Also, this should really be called "revisit this SCC".
----------------
Can you please exapnd this comment a little bit more?

================
Comment at: include/llvm/Transforms/IPO/ArgumentPromotion.h:19-20
@@ +18,4 @@
+struct ArgumentPromotionPass : PassInfoMixin<ArgumentPromotionPass> {
+  /// The maximum number of elements to expand, or 0 for unlimited.
+  unsigned MaxElements = 3;
+  PreservedAnalyses run(CallGraphSCC &C, CGSCCAnalysisManager &AM);
----------------
maybe this can be private?

================
Comment at: include/llvm/Transforms/IPO/PruneEH.h:18
@@ +17,3 @@
+
+struct PruneEHPass : PassInfoMixin<PruneEHPass> {
+  PreservedAnalyses run(CallGraphSCC &C, CGSCCAnalysisManager &AM);
----------------
Do you mind to put a doxygen comment briefly explaining what this struct/class does?

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1027
@@ -1081,2 +1026,3 @@
     Function *F = I->getFunction();
-    if (!F || F->hasFnAttribute(Attribute::OptimizeNone)) {
+    // XXX: The isDeclaration check here is because in the new PM BasicAA
+    // uses DomTree which will cause it to crash on declarations.
----------------
XXX -> FIXME? Here and everywhere else in the patch where it applies

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1084
@@ +1083,3 @@
+
+  return runImpl(C, AARGetter) ? PreservedAnalyses::none()
+                               : PreservedAnalyses::all();
----------------
This preserves the CFG, add a FIXME etc etc...


http://reviews.llvm.org/D21921