-
Notifications
You must be signed in to change notification settings - Fork 10.5k
EscapeAnalysis: add a refcount flag to content nodes. #28153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Only the second commit is new here. It depends on this PR awaiting review |
@swift-ci test |
@swift-ci test source compatibility |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Although I didn't understand the implications of all the details.
For me, the algorithms - especially initializePointsTo and mergeAllScheduledNodes - are hard to think through, compared to the original version (which might be a natural thing, because I implemented the original version :-)).
Anyway, it's another indication to me that we should massively simplify escape analysis, e.g. by removing defer edges and enforcing a struct ref - address graph structure (as we talked).
I think you should land this changes now - and hopefully there are not many bugs introduced which this big change - and eventually we should work on simplifying EA.
return Traversal::Follow; | ||
}); | ||
} | ||
// For all nodes visited in step 1, if any node was not backward-reachable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for handling cycles which don't have a points-to edge, right? Like this was done in the old version in updatePointsTo inside ' if (isInitialSet) { }'
If yes, can you add a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really about cycles, which is one of the things that confused me about the old code. I still don't like this initializePointsTo
code because it's still hard to figure out from a quick read. The concept is nice though:
Step 1: find all nodes in the uninitialized defer web and mark which ones already have edges.
Step 2: A direct application of the graph invariant "defer paths end in a pointsTo edge"
2a: walk backward from the edges checking that all the other nodes are reachable.
2b: add an edge to one of the remaining nodes and repeat
I was trying to be super efficient about this initially, then added extra intelligence later in (2b) to pick the best remaining node without changing the algorithmic complexity. But in hindsight it would be easier to understand the code if instead step (2b) just iterated over the remaining nodes again and directly identified the "leaves" in the graph.
I think I'll avoid rewriting it again in case we're able to throw it all away instead.
@swift-ci test |
Build failed |
Build failed |
@swift-ci test |
Build failed |
Build failed |
This property will allow alias analysis to be safely optmistic when querying the connection graph. A node that is known to have a ref count is know to keep alive everything it points to. Therefore, calling a deinitializer on a different reference cannot release the RC node's contents.
@swift-ci test |
Build failed |
Build failed |
This property will allow alias analysis to be safely optmistic when
querying the connection graph. A node that is known to have a ref
count is know to keep alive everything it points to. Therefore,
calling a deinitializer on a different reference cannot release the RC
node's contents.