This is an archive of the discontinued LLVM Phabricator instance.

[mlgo] Fix accounting for SCC splits
ClosedPublic

Authored by Northbadge on Jun 13 2022, 1:46 PM.

Details

Summary

Previously if the inliner split an SCC such that an empty one remained, the MLInlineAdvisor could potentially lose track of the EdgeCount if a subsequent CGSCC pass modified the calls of a function that was initially in the SCC pre-split. Saving the seen nodes in onPassEntry resolves this.

Diff Detail

Event Timeline

Northbadge created this revision.Jun 13 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 1:46 PM
Northbadge requested review of this revision.Jun 13 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 1:46 PM

Sync with parent change

lg, some nits

llvm/lib/Analysis/MLInlineAdvisor.cpp
182

point out we're reusing NodesInLastSCC for that, so it's clear that this is the intent: 1) drain it (lines 160-180), 2) remember.

I'd even add an assert(NodesInLastSCC.empty()) right above the for loop - same reason, clarifies the design intent

Finally: I think you can do NodesInLastSCC.insert(LastSCC.begin(), LastSCC.end()) or smth like that (skipping the for)

211

nit: you can do this:

auto I = NodesInLastSCC.insert(&N)
if (I.second)
  EdgedOfLastSeenNodes +=...

this way the lookup is done once

Northbadge marked 2 inline comments as done.

Resolved comments. Thanks for the quick review!

Northbadge added inline comments.Jun 14 2022, 2:26 PM
llvm/lib/Analysis/MLInlineAdvisor.cpp
182

got it! but using SCC iterators won't work since we're inserting node pointers here, while the iterators would return nodes

mtrofin accepted this revision.Jun 14 2022, 5:49 PM
mtrofin added inline comments.
llvm/lib/Analysis/MLInlineAdvisor.cpp
182

ah, ok

This revision is now accepted and ready to land.Jun 14 2022, 5:49 PM
Northbadge marked an inline comment as done.Jun 14 2022, 7:58 PM

Rebase to upstream main

This comment was removed by Northbadge.

Actually rebase to upstream main

This revision was landed with ongoing or failed builds.Jun 15 2022, 10:54 AM
This revision was automatically updated to reflect the committed changes.