This is an archive of the discontinued LLVM Phabricator instance.

[PM][CGSCC] Add a helper to update the call graph from SCC passes
ClosedPublic

Authored by jdoerfert on Dec 30 2019, 7:16 PM.

Details

Summary

With this patch new trivial edges can be added to an SCC in a CGSCC
pass via the updateCGAndAnalysisManagerForCGSCCPass method. It shares
almost all the code with the existing
updateCGAndAnalysisManagerForFunctionPass method but it implements the
first step towards the TODOs.

This was initially part of D70927.

Diff Detail

Event Timeline

jdoerfert created this revision.Dec 30 2019, 7:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2019, 7:16 PM

Unit tests: pass. 61159 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

My patch 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 relies on being able to add ref edges, so I want to make sure that doesn't break :)

llvm/include/llvm/Analysis/CGSCCPassManager.h
421

nit: git grep "CG-SCC" only turns up matches from this patch and D70927. I think "CGSCC" is the canonical abbreviation.

llvm/lib/Analysis/CGSCCPassManager.cpp
633

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.

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
1285

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.

1365

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?

1422

Same as above, this sentence trails off. Tests TestUpdateCGAndAnalysisManagerForPasses2 and TestUpdateCGAndAnalysisManagerForPasses3 are testing inserting call edges across SCCs, is that right?

wenlei added a subscriber: wenlei.Jan 5 2020, 2:59 PM
jdoerfert updated this revision to Diff 236473.Jan 6 2020, 3:23 PM

First add new edges before the SCC might get split up

jdoerfert updated this revision to Diff 236476.Jan 6 2020, 3:34 PM
jdoerfert marked 7 inline comments as done.

Addressed comments

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
1365

Yes, correct. The sentence is not an assertion but a regular expression that checks for the assertion.

1422

The first tests add edges inside the SCC, the second inside the RefSCC but from an outer SCC to an inner one. Both are trivial so we should be all right adding them and then calling updateCGAndAnalysisManagerForCGSCCPass.

Unit tests: pass. 61185 tests passed, 0 failed and 729 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61185 tests passed, 0 failed and 729 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

modocache added inline comments.Jan 6 2020, 7:34 PM
llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
1365

Aha! Thanks, I didn't know that.

modocache added inline comments.Jan 8 2020, 3:21 PM
llvm/lib/Analysis/CGSCCPassManager.cpp
498

You'll need to add a (void)TargetRC; statement or something here and on similar lines. In release builds, I'm seeing the following warnings:

llvm/lib/Analysis/CGSCCPassManager.cpp:498:13: warning: unused variable 'TargetRC' [-Wunused-variable]
    RefSCC &TargetRC = TargetC.getOuterRefSCC();
            ^
llvm/lib/Analysis/CGSCCPassManager.cpp:508:13: warning: unused variable 'TargetRC' [-Wunused-variable]
    RefSCC &TargetRC = TargetC.getOuterRefSCC();
            ^
jdoerfert marked an inline comment as done.Jan 8 2020, 8:24 PM
jdoerfert added inline comments.
llvm/lib/Analysis/CGSCCPassManager.cpp
498

Will do, thanks. (I add WITH_ASSERTS to my release build so I don't see these warnings).

jdoerfert retitled this revision from [PM][CG-SCC] Add a helper to update the call graph from SCC passes to [PM][CGSCC] Add a helper to update the call graph from SCC passes.Jan 31 2020, 1:26 PM
jdoerfert edited the summary of this revision. (Show Details)
jdoerfert updated this revision to Diff 241838.Jan 31 2020, 4:17 PM

Addressed comment

Unit tests: pass. 62376 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

JonChesterfield accepted this revision.Feb 1 2020, 3:13 AM

Change looks great to me. Straightforward, removes FIXME notes, has associated tests, already picked up a dependency. Thanks!

This revision is now accepted and ready to land.Feb 1 2020, 3:13 AM
This revision was automatically updated to reflect the committed changes.