Skip to content

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

Merged
merged 1 commit into from
Nov 13, 2019
Merged

EscapeAnalysis: add a refcount flag to content nodes. #28153

merged 1 commit into from
Nov 13, 2019

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Nov 8, 2019

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.

@atrick atrick requested a review from eeckstein November 8, 2019 19:42
@atrick
Copy link
Contributor Author

atrick commented Nov 8, 2019

Only the second commit is new here.

It depends on this PR awaiting review
#28139

@atrick
Copy link
Contributor Author

atrick commented Nov 11, 2019

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Nov 11, 2019

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0f40db8f18e2f93324ddf07975111861318a0eff

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0f40db8f18e2f93324ddf07975111861318a0eff

Copy link
Contributor

@eeckstein eeckstein left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

@atrick atrick Nov 12, 2019

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.

@atrick
Copy link
Contributor Author

atrick commented Nov 12, 2019

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 29ca91ab4c5bf64d4bae27329cc59ef7cd90f3e1

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 29ca91ab4c5bf64d4bae27329cc59ef7cd90f3e1

@atrick
Copy link
Contributor Author

atrick commented Nov 13, 2019

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 29ca91ab4c5bf64d4bae27329cc59ef7cd90f3e1

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 29ca91ab4c5bf64d4bae27329cc59ef7cd90f3e1

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.
@atrick
Copy link
Contributor Author

atrick commented Nov 13, 2019

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8def5ccb41e4371112b54b0a0b6d4c4ff41afce8

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8def5ccb41e4371112b54b0a0b6d4c4ff41afce8

@atrick atrick merged commit 166daeb into swiftlang:master Nov 13, 2019
@atrick atrick deleted the escape-refcount branch December 23, 2019 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants