[PATCH] D72025: [PM][CG-SCC] Add a helper to update the call graph from SCC passes

[Brian Gesiak via Phabricator via llvm-commits]

modocache added a comment.

My patch D71899 <https://reviews.llvm.org/D71899> makes use of the API introduced in this patch (it's adds an outlining CGSCC pass to the new pass manager pipeline), and it works great! Thanks for this.

I'm not an expert in this area, so maybe @chandlerc should review this before this is committed. I had mostly nit-picks.

Also, one suggestion: the unit tests for this patch are great! As far as I can tell, they all test adding call edges. Could you add some for adding ref edges? I think that would improve the test coverage here, but also my patch D71899 <https://reviews.llvm.org/D71899> relies on being able to add ref edges, so I want to make sure that doesn't break :)



================
Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:421
 
+/// Helper to update the call graph after running a CG-SCC pass.
+///
----------------
nit: `git grep "CG-SCC"` only turns up matches from this patch and D70927. I think "CGSCC" is the canonical abbreviation.


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:613
+    assert(RC == &TargetRC ||
+           RC->isAncestorOf(TargetRC) && "New call edge is not trivial!");
+    RC->insertTrivialRefEdge(N, *CallTarget);
----------------
Small nitpick: This should be `"new ref edge is not trivial"`, I think. The variable names, such as `Node *CallTarget`, could also be updated to refer to ref edges, not call edges.


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:1285
+
+#ifndef NDEBUG
+
----------------
At first I wasn't sure why these tests were guarded by this, but as I read below I could see that they're primarily testing that `updateCGAndAnalysisManagerFor{Function,CGSCC}Pass` does not trigger an assert (and those would only fire if `NDEBUG` wasn't defined). I'm relatively new to this project so maybe it's just me, or maybe a comment here might be helpful.


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:1363
+    ASSERT_DEATH(updateCGAndAnalysisManagerForFunctionPass(CG, C, H2N, AM, UR),
+                 "Any new calls should be modeled as");
+  }));
----------------
I think this sentence is missing its ending. Basically `TestUpdateCGAndAnalysisManagerForPasses0` is testing that CGSCC passes can insert new edges, and `TestUpdateCGAndAnalysisManagerForPasses1` is testing that function passes cannot -- is that right?


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:1420
+    ASSERT_DEATH(updateCGAndAnalysisManagerForFunctionPass(CG, C, FN, AM, UR),
+                 "Any new calls should be modeled as");
+  }));
----------------
Same as above, this sentence trails off. Tests `TestUpdateCGAndAnalysisManagerForPasses2` and `TestUpdateCGAndAnalysisManagerForPasses3` are testing inserting call edges across SCCs, is that right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72025/new/

https://reviews.llvm.org/D72025