This is an archive of the discontinued LLVM Phabricator instance.

[CallGraphUpdater] Remove nodes from their SCC (old PM)
ClosedPublic

Authored by jdoerfert on Apr 9 2020, 8:05 PM.

Details

Summary

We can and should remove deleted nodes from their respective SCCs. We
did not do this before and this was a potential problem even though I
couldn't locally trigger an issue. Since the DeleteNode would assert
if the node was not in the SCC, we know we only remove nodes from their
SCC and only once (when run on all the Attributor tests).

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 9 2020, 8:05 PM
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
hfinkel accepted this revision.Apr 12 2020, 7:29 AM

LGTM

llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
108

I realize that this probably doesn't matter, but I wonder if the code would be clearer here if you wrote:

if (CG && ...)

because you've dereferencing CG here and use of CGSCC is guarded below by GC (instead of directly by checking CGSCC). Keeping the guarding conditions the same should make the code more readable by eliminating potential questions about why they're different.

This revision is now accepted and ready to land.Apr 12 2020, 7:29 AM
MaskRay added inline comments.
llvm/lib/Analysis/CallGraphSCCPass.cpp
566

clang-format prefers /*New=*/nullptr

jdoerfert updated this revision to Diff 257890.Apr 15 2020, 4:30 PM
jdoerfert marked 2 inline comments as done.

addressed nits

This revision was automatically updated to reflect the committed changes.