This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][CGSCC] Handle newly added functions not directly referenced by existing nodes
AbandonedPublic

Authored by aeubanks on Oct 1 2020, 6:25 PM.

Details

Summary
As mentioned in https://reviews.llvm.org/D87798, newly created functions
that aren't referenced by an existing node are not properly handled.

This checks all newly added functions for references to other functions
that don't have a populated node.

Coroutines can split a function into multiple functions. For example,
foo can be split into foo, foo.1, and foo.2. foo may reference foo.1,
but foo.2 is only referenced by foo.1.
coro-retcon-resume-values2.ll actually has an instance of this
happening, which would trigger an assert that a node is unpopulated.

Diff Detail

Event Timeline

aeubanks created this revision.Oct 1 2020, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 6:25 PM
aeubanks requested review of this revision.Oct 1 2020, 6:25 PM
MaskRay added inline comments.
llvm/lib/Analysis/CGSCCPassManager.cpp
981–992

The whole loop deserves a comment, along the line of: the newly created nodes are referenced by some SCC nodes via call/ref edges. We conservatively assume the newly created nodes are part of the SCC (as if they call back to the SCC).

983

assert(!NewNode->isPopulated())

995

Having the container in the loop can lead to duplicate traversal. Move NewNodesWorklist & NewNodesVisited outside the loop (and decrease 16 -> 4 may be sufficient)

Additionally, should NewNodesVisited affect the G.getLibFunctions() loop below which uses Visited?

coro-retcon-resume-values2.ll actually has an instance of this happening.

Might be better to elaborate this a bit.

aeubanks updated this revision to Diff 311290.Dec 11 2020, 11:45 AM

rebase
add comments

aeubanks added inline comments.Dec 11 2020, 11:49 AM
llvm/lib/Analysis/CGSCCPassManager.cpp
981–992

Added comments, better?

983

There might be multiple references to a new node added into NewNodes before we reach the new node.

995

Each iteration will traverse a different function. The only potential speedup is if multiple functions reference the same Constant.

The G.getLibFunctions() part below is only relevant to the original function in N, doesn't affect the other functions we are traversing here.

aeubanks edited the summary of this revision. (Show Details)Dec 11 2020, 11:49 AM
aeubanks planned changes to this revision.Dec 16 2020, 4:00 PM

Actually I think I need to better handle newly created functions. Currently I'm putting them in the same SCC as the current node, but in reality it might need to be in its own SCC (else we'd break some invariants about all nodes being able to reach all other nodes in the same SCC)

aeubanks abandoned this revision.Dec 26 2020, 1:57 PM

https://reviews.llvm.org/D93828 is a more principled way of fixing this