EscapeAnalysis: rework graph update and merge algorithms #28139

atrick · 2019-11-08T00:54:25Z

Here is a change-by-change description in diff order:

Replace updatePointsTo with initializePointsTo and
mergePointsTo. Merging is very simple on its own. Initialization
requires some extra consideration for graph invariants. This
separation makes it possible to write stong asserts and to
independently reason about the correctness of each step based on
static invariants.

Replace getContentNode with createContentNode, and add two higher
level APIs createMergedContent, and getFieldContent. This makes
explicit the important cases of merging nodes and creating a special
nodes for class fields. This slightly simplifies adding properties to
content nodes and helps understand the structure of the graph.

Factor out an escapeContentsOf helper for use elsewhere...

Add a getValueContent helper. This is where we can tie the
properties of content nodes to the address values that are used to
address that content. This now also ensures that a Value node's
value field is consistent with all SILValues that map to it.

Add -escapes-internal-verify to check that the graph is in a valid
state after every merge or update step. This verification drove the
partial rewrite of mergeAllScheduledNodes.

ConnectionGraph::defer implementation: explictly handle the three
possible cases of pointsTo initialization or pointsTo merging at the
top level, so that those underlying implementations do not need to
dynamically handle weirdly different scenarios.

ConnectionGraph::initializePointsTo implementation: this simplified
implementation is possible by relying on invariants that can be
checked at each merge/update step. The major functional difference is
that it avoids creating unnecessary pointsTo edges. The previous
implementation often created pointsTo edges when adding defer edges
just to be conservative. Fixing this saved my sanity during debugging
because the pointsTo edges now always correspond to a SIL operations
that dereference the pointer. I'm also arguing without evidence that
this should be much more efficient.

ConnectionGraph::mergeAllScheduledNodes implementation: Add
verification to each step so that we can prove the other utilities
that are used while merging aren't making incorrect assumptions about
the graph state. Remove checks for merged nodes now that the graph is
consistently valid. Also remove a loop at the end that didn't seem to
do anything. The diff is impossible to review, but the idea is
basically the same. As long as it's still possible to scan through the
steps in the new code without getting totally lost, then the goal was
achieved.

ConnectionGraph::mergePointsTo: This is extremely simple now. In all
the places where we used to call updatePointsTo, and now call
mergePointsTo, it's a lot easier for someone debugging the code to
reason about what could possibly happen at that point.

createMergedContent is a placeholder for transferring node properties.

The getFieldContent helper may seem silly, but I find it helpful to
see all the important ways that content can be created in one place
next to the createContentNode, and I like the way that the creation of
the special "field content" node is more explicit in the source.

ConnectionGraph::mergeFrom implementation: this is only a minor
cleanup to remove some control flow nesting and use the CGNodeWorklist
abstraction.

In AnalyzeInstruction, add EscapeAnalysis::getValueContent helper. It
eliminates an extra step of going through the value node to get at its
content node. This is where we can derive content node properties from
the SILValue that dereferences the content. We can update the content
node's associated value 'V' if it's useful. It's also a place to put
assertions specific to the first level of content.

In AnalyzeInstruction, Array semantic calls: add support for
getValueContent so we can derive node properties. This is also nice
because it's explicit about which nodes are value content vs. field
content.

In AnalyzeInstruction, cleanup Release handling: use the explicit
APIs: getValueContent, getFieldContent, and escapeContentsOf.

In AnalyzeInstruction, assert that load-like things can't produce addresses.

In AnalyzeInstruction, add comments to clarify object projection handling.

In AnalyzeInstruction, add comments to explain store handling.

In AnalyzeInstruction, drop the assumption that all partial applies hold pointers.

In AnalyzeInstruction, handle aggregates differently so that Value
nodes are always consistent with their SILValue and can be
verified. Aggregates nodes are still coalesced if they only have a
single pointer-type subelement. If we arbitrarily coalesced an
aggregate with just one of its subelements then there would be no
consistent way to identify the value that corresponds to a connection
graph node.

atrick · 2019-11-08T00:59:33Z

@eeckstein This PR has the bulk of the underlying changes to EscapeAnalysis itself. This covers the remaining issues that tripped me up during debugging.

After this, adding a refcount attribute to nodes is trivial!

It will be annoying to review. I realize it should have been at least two PRs, but it's getting challenging to keep breaking things up and rebasing. In fact, a few test failures appeared in today's rebase that I'm still debugging.

All remaining functional changes in subsequent PRs will be confined to the alias analysis queries.

atrick · 2019-11-08T17:36:46Z

@eeckstein The two major take aways from this patch are:

(1) Impose graph structure and reduce superfluous nodes and edges.

Incrementally make the connection graph and the APIs used to construct
it more structured.

_This allows node properties based on the SILValue to be reliably added to nodes_

Although that was the initial motiviation, there are other
benefits. Non-content nodes now have verifiable SILValues. Content
nodes now have meaningful SILValues even though they can't be
guaranteed due to merging. As a result it is *much* easier to debug
the graph and correlate it with the SIL. Rather than a web of
connection graph nodes with no identity and edges that don't
correspond to anything in SIL, the graph nodes now have value number
that correspond to the instruction used to dereference the node. The
edges also exhibit structure now. A pointsTo edge now (in practice)
always corresponds to a real posinter deference in the SIL. Doing this
required more than just adding some helpers, it was also necessary to
rewrite the graph merge and update algorithms.

(2) Split up underlying functionality into more explicit steps

Breaks apart the most complex parts of the graph algorithms into small
self-contained, self-checked steps. The purpose of each step is clear
and it's possible to reason about correctness from basic
invariants. Each merge step can now run full graph verification.

This was also done to move toward an invariant that the graph is never
mutated during a query. But to finish that goal, we need to add a
use-point query. With that, there will be no node creation, use point
propagation, new defer edges, etc. after graph building. At the very
least, this will make it sane to debug the output of the analysis.

atrick · 2019-11-11T02:18:23Z

@swift-ci test

atrick · 2019-11-11T02:18:36Z

@swift-ci benchmark

atrick · 2019-11-11T02:18:46Z

@swift-ci test source compatibility

swift-ci · 2019-11-11T02:20:45Z

Build failed
Swift Test Linux Platform
Git Sha - 499b5e136666f409910532e5c1134ea206c47c84

swift-ci · 2019-11-11T02:20:46Z

Build failed
Swift Test OS X Platform
Git Sha - 499b5e136666f409910532e5c1134ea206c47c84

swift-ci · 2019-11-11T03:06:08Z

Performance: -O

Improvement	OLD	NEW	DELTA	RATIO
FlattenListLoop	3220	2790	-13.4%	1.15x (?)

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendOptionals	1510	590	-60.9%	2.56x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStubToNSDate2	970	1050	+8.2%	0.92x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

atrick · 2019-11-11T22:14:09Z

@swift-ci test

atrick · 2019-11-11T22:15:32Z

@swift-ci test

swift-ci · 2019-11-11T22:19:42Z

Build failed
Swift Test OS X Platform
Git Sha - b51df9e712f6c39c2a437ca5017de83fc1c1bcdb

swift-ci · 2019-11-11T22:44:34Z

Build failed
Swift Test Linux Platform
Git Sha - b51df9e712f6c39c2a437ca5017de83fc1c1bcdb

atrick · 2019-11-12T00:46:24Z

@swift-ci test OS X platform

atrick · 2019-11-12T01:33:16Z

@swift-ci benchmark

swift-ci · 2019-11-12T02:00:43Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	4538	4926	+8.6%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
FlattenListLoop	2920	2703	-7.4%	1.08x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
SuffixCountableRange	4	6	+50.0%	0.67x (?)

Improvement	OLD	NEW	DELTA	RATIO
PrefixArray	14	13	-7.1%	1.08x (?)

Code size: -Osize

Performance: -Onone

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendGenericStructs	1320	630	-52.3%	2.10x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

swift-ci · 2019-11-12T02:49:57Z

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

atrick · 2019-11-12T03:24:06Z

PR testing repeatedly fails on SwiftSyntax which doesn't appear to be related to this PR.

eeckstein

See my comments in #28153 (review)

The two major take aways from this patch are: (1) Impose graph structure and reduce superfluous nodes and edges. Incrementally make the connection graph and the APIs used to construct it more structured. _This allows node properties based on the SILValue to be reliably added to nodes_ Although that was the initial motiviation, there are other benefits. Non-content nodes now have verifiable SILValues. Content nodes now have meaningful SILValues even though they can't be guaranteed due to merging. As a result it is *much* easier to debug the graph and correlate it with the SIL. Rather than a web of connection graph nodes with no identity and edges that don't correspond to anything in SIL, the graph nodes now have value number that correspond to the instruction used to dereference the node. The edges also exhibit structure now. A pointsTo edge now (in practice) always corresponds to a real pointer deference in the SIL. Doing this required more than just adding some helpers, it was also necessary to rewrite the graph merge and update algorithms. (2) Split up underlying functionality into more explicit steps Breaks apart the most complex parts of the graph algorithms into small self-contained, self-checked steps. The purpose of each step is clear and it's possible to reason about correctness from basic invariants. Each merge step can now run full graph verification. This was also done to move toward an invariant that the graph is never mutated during a query. But to finish that goal, we need to add a use-point query. With that, there will be no node creation, use point propagation, new defer edges, etc. after graph building. At the very least, this will make it sane to debug the output of the analysis. --- Here is a change-by-change description in diff order: Replace `updatePointsTo` with `initializePointsTo` and `mergePointsTo`. Merging is very simple on its own. Initialization requires some extra consideration for graph invariants. This separation makes it possible to write stong asserts and to independently reason about the correctness of each step based on static invariants. Replace `getContentNode` with `createContentNode`, and add two higher level APIs `createMergedContent`, and `getFieldContent`. This makes explicit the important cases of merging nodes and creating a special nodes for class fields. This slightly simplifies adding properties to content nodes and helps understand the structure of the graph. Factor out an `escapeContentsOf` helper for use elsewhere... Add a `getValueContent` helper. This is where we can tie the properties of content nodes to the address values that are used to address that content. This now also ensures that a Value node's value field is consistent with all SILValues that map to it. Add -escapes-internal-verify to check that the graph is in a valid state after every merge or update step. This verification drove the partial rewrite of mergeAllScheduledNodes. ConnectionGraph::defer implementation: explictly handle the three possible cases of pointsTo initialization or pointsTo merging at the top level, so that those underlying implementations do not need to dynamically handle weirdly different scenarios. ConnectionGraph::initializePointsTo implementation: this simplified implementation is possible by relying on invariants that can be checked at each merge/update step. The major functional difference is that it avoids creating unnecessary pointsTo edges. The previous implementation often created pointsTo edges when adding defer edges just to be conservative. Fixing this saved my sanity during debugging because the pointsTo edges now always correspond to a SIL operations that dereference the pointer. I'm also arguing without evidence that this should be much more efficient. ConnectionGraph::mergeAllScheduledNodes implementation: Add verification to each step so that we can prove the other utilities that are used while merging aren't making incorrect assumptions about the graph state. Remove checks for merged nodes now that the graph is consistently valid. Also remove a loop at the end that didn't seem to do anything. The diff is impossible to review, but the idea is basically the same. As long as it's still possible to scan through the steps in the new code without getting totally lost, then the goal was achieved. ConnectionGraph::mergePointsTo: This is extremely simple now. In all the places where we used to call updatePointsTo, and now call mergePointsTo, it's a lot easier for someone debugging the code to reason about what could possibly happen at that point. `createMergedContent` is a placeholder for transferring node properties. The `getFieldContent` helper may seem silly, but I find it helpful to see all the important ways that content can be created in one place next to the createContentNode, and I like the way that the creation of the special "field content" node is more explicit in the source. ConnectionGraph::mergeFrom implementation: this is only a minor cleanup to remove some control flow nesting and use the CGNodeWorklist abstraction. In AnalyzeInstruction, add EscapeAnalysis::getValueContent helper. It eliminates an extra step of going through the value node to get at its content node. This is where we can derive content node properties from the SILValue that dereferences the content. We can update the content node's associated value 'V' if it's useful. It's also a place to put assertions specific to the first level of content. In AnalyzeInstruction, Array semantic calls: add support for getValueContent so we can derive node properties. This is also nice because it's explicit about which nodes are value content vs. field content. In AnalyzeInstruction, cleanup Release handling: use the explicit APIs: getValueContent, getFieldContent, and escapeContentsOf. In AnalyzeInstruction, assert that load-like things can't produce addresses. In AnalyzeInstruction, add comments to clarify object projection handling. In AnalyzeInstruction, add comments to explain store handling. In AnalyzeInstruction, drop the assumption that all partial applies hold pointers. In AnalyzeInstruction, handle aggregates differently so that Value nodes are always consistent with their SILValue and can be verified. Aggregates nodes are still coalesced if they only have a single pointer-type subelement. If we arbitrarily coalesced an aggregate with just one of its subelements then there would be no consistent way to identify the value that corresponds to a connection graph node.

atrick · 2019-11-12T23:31:32Z

@swift-ci test and merge

atrick · 2019-11-12T23:55:13Z

@swift-ci test

atrick requested a review from eeckstein November 8, 2019 00:55

atrick mentioned this pull request Nov 8, 2019

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

Merged

eeckstein approved these changes Nov 12, 2019

View reviewed changes

atrick merged commit 95c716c into swiftlang:master Nov 13, 2019

atrick deleted the escape-createcontentnode branch December 23, 2019 03:13

EscapeAnalysis: rework graph update and merge algorithms #28139

EscapeAnalysis: rework graph update and merge algorithms #28139

Uh oh!

Conversation

atrick commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Nov 8, 2019

Uh oh!

atrick commented Nov 8, 2019

Uh oh!

atrick commented Nov 11, 2019

Uh oh!

atrick commented Nov 11, 2019

Uh oh!

atrick commented Nov 11, 2019

Uh oh!

swift-ci commented Nov 11, 2019

Uh oh!

swift-ci commented Nov 11, 2019

Uh oh!

swift-ci commented Nov 11, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

atrick commented Nov 11, 2019

Uh oh!

atrick commented Nov 11, 2019

Uh oh!

swift-ci commented Nov 11, 2019

Uh oh!

swift-ci commented Nov 11, 2019

Uh oh!

atrick commented Nov 12, 2019

Uh oh!

atrick commented Nov 12, 2019

Uh oh!

swift-ci commented Nov 12, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

swift-ci commented Nov 12, 2019

Uh oh!

atrick commented Nov 12, 2019

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

atrick commented Nov 12, 2019

Uh oh!

atrick commented Nov 12, 2019

Uh oh!

Uh oh!

atrick commented Nov 8, 2019 •

edited

Loading