EscapeAnalysis: add node flags and change the meaning of "escaping" #28800
Conversation
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
@swift-ci benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Build failed |
|
Build failed |
There was a problem hiding this comment.
Basically LGTM.
But I'm wondering if it's worth adding the complexity of may/must-have-reference.
I think it's better to strictly distinguish between reference and address-or-pointer nodes. That would mean that those two types of nodes are never merged. To achieve this it's probably enough to treat raw_pointer_to_ref conservatively.
| /// base object for a SIL memory access, which may look through | ||
| /// ref_element_addr, if the access is to this node's pointsTo content, then | ||
| /// the access' base object must also be mapped to the pointsTo node, not | ||
| /// this node. |
There was a problem hiding this comment.
I can guess what "isInteriorFlag" means, but after reading this comment, I'm confused.
There was a problem hiding this comment.
It's surprisingly hard to put into words:
/// True if this node's pointsTo content is part of the same object with
/// respect to SIL memory access. When this is true, then this node must
/// have a content node.
///
/// When this flag is false, it provides a "separate access guarantee" for
/// stronger analysis. If the base object for a SIL memory access is mapped
/// to this node, then the accessed memory must have the same escaping
/// properties as the base object. In contrast, when this is true, the
/// connection graph may model the escaping properties of the base object
/// separately from the accessed memory.
|
@eeckstein Also, I think it's fine to call the "reference" kind So, there are only three kinds of pointers: NoPointer (don't create a node) ReferenceOnly (safe to make normal assumptions) AnyPointer (may have addresses, rawpointers, or any mix of thoses with references) You might be suggesting that we have a NonReferencePointer instead, which we know doesn't contain references, but that doesn't simplify any of the analysis. I don't know how to prevent different kinds of nodes from being merged. When I first started working on this I wanted to assert that merges always obeyed/preserved the graph properties. I only realized that was hopeless after a very long time debugging. Maybe there's an alternate world where we only ever create content nodes for references. I'm not sure what we'd be giving up to gain that. We have bridge object casting, which is probably on the critical path, and rawpointer fields in objects that also have references. The easiest way for me to guarantee that we don't need to always assume worst case given the possibility of non-reference pointers is just to recognize the nodes that can't contain those bad things and conservatively flag and merge them. There's another issue of handling a null content node, where, without a flag, we'd always need to assume the worst. The missing node could be because the content was never accessed (optimistic) or because the pointer value contained or merged with some non-reference pointer value (pessimistic). |
Categorize three kinds of pointers: NoPointer (don't create a node) ReferenceOnly (safe to make normal assumptions) AnyPointer (may have addresses, rawpointers, or any mix of thoses with references) Flag ConnectionGraph nodes as - hasReferenceOnly - isInterior An interior node always has an additional content node. All sorts of arbitrary node merging is supported. Nodes with totally different properties can be safely merged. Interior nodes can safely be merged with their field content (which does happen surprisingly often). Alias analysis will use these flags to safely make assumptions about properties of the connection graph.
…ent node only. For alias analysis query to be generally correct, we need to effectively merge the escape state and use points for everything in a defer web. It was unclear from the current design whether the "escaping" property applied to the pointer value or its content. The implementation is inconsistent in how it was treated. It appears that some bugs have been worked around by propagating forward through defer edges, some have been worked around by querying the content instead of the pointer, and others have been worked around be creating fake use points at block arguments. If we always simply query the content for escape state and use points, then we never need to propagate along defer edges. The current code that propagates escape state along defer edges in one direction is simply incorrect from the perspective of alias analysis. One very attractive solution is to merge nodes eagerly without creating any defer edges, but that would be a much more radical change even than what I've done here. It would also pose some new issues: how to resolve the current "node types" when merging and how to deal with missing content nodes. This solution of applying escape state to content nodes solves all these problems without too radical of a change at the expense of eagerly creating content nodes. (The potential graph memory usage is not really an issue because it's possible to drastically shrink the size of the graph anyway in a future commit--I've been able to fit a node within one cache line). This solution nicely preserves graph structure which makes it easy to debug and relate to the IR. Eagerly creating content nodes also solves the missing content node problem. For example, when querying canEscapeTo, we need to know whether to look at the escape state for just the pointer value itself, or also for its content. It may be possible the its content node is actually part of the same object at the IR level. If the content node is missing, then we don't know if the object's interior address is not recognizable/representable or whether we simply never saw an access to the interior address. We can't simply look at whether the current IR value happens to be a reference, because that doesn't tell us whether the graph node may have been merged with a non-reference node or even with it's own content node. To be correct in general, this query would need to be extremely conservative. However, if content nodes are always created for references, then we only need to query the escape state of a pointer's content node. The content node's flag tells us if it's an interior node, in which case it will always point to another content node which also needs to be queried.
That appears to have been a partial workaround for the real problem that usepoints need to be propagated across the entire defer web. This is now solved by considering use points on the reference node's content, not the reference node itself.
atrick
changed the title
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
The type safety of SIL should ensure that we don't merge reference and address/pointer types. Of course, as long as we treat instructions, which convert between the both, conservatively. |
|
@eeckstein right, we could treat all casts that change pointer type as a completely separate value. But that includes bridge object casts. I'm always nervous that we won't recognize some array/dictionary pattern. Also, before this we didn't have a solution for objects that contain both reference and raw pointer fields. At any rate, this way we don't give up any precision in the graph, we just lose the node flags which makes AliasAnalysis a bit more conservative in that case. To be a little more clear: If we ever have a "content" node that represents a raw pointer, then we ca run into correctness issues if we don't have these flags. So, for example, if we ever load a raw pointer or load from an object containing a raw pointer then we end up with a content node that may be merged arbitrarily with other nodes. I'm operating under the rule that it should be safe to merge any two content nodes regardless of where they came from. Every time I thought I could make assumptions about what could be merged, I was proven wrong once I put the assertions in place. |
Correctness: do not make any unenforced assumptions about how the connection graph is built (I don't think the previous assumption about the structure of the graph node mapped to a reference-type value would always hold if content nodes can be arbitrarily merged). Only make one assumption about the client code: the access being checked must be to some address within the provided value, not another object indirectly reachable from that value. Optimization: Allow escape analysis to prove that an addressable object does not escape even when one of its reference-type fields escapes.
Fix tests for changes to EscapeAnalysis output - New node flags - Only content nodes are marked escaping - Content nodes are eagerly created - No defer edges for block args
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
Build failed |
|
Build failed |
|
okay |
I've broken this PR loosely into commits. They can't quite be tested independently, but it does help for the sake of git log and annotate.
EscapeAnalysis: Add PointerKind and interior/reference flags
Categorize three kinds of pointers:
NoPointer (don't create a node)
ReferenceOnly (safe to make normal assumptions)
AnyPointer (may have addresses, rawpointers, or any mix of thoses
with references)
Flag ConnectionGraph nodes as
An interior node always has an additional content node.
All sorts of arbitrary node merging is supported. Nodes with totally
different properties can be safely merged. Interior nodes can safely
be merged with their field content (which does happen surprisingly
often).
Alias analysis will use these flags to safely make assumptions about
properties of the connection graph.
EscapeAnalysis: Make EscapeState and UsePoints a property of the
content node only.
For alias analysis query to be generally correct, we need to
effectively merge the escape state and use points for everything in a
defer web.
It was unclear from the current design whether the "escaping" property
applied to the pointer value or its content. The implementation is
inconsistent in how it was treated. It appears that some bugs have
been worked around by propagating forward through defer edges, some
have been worked around by querying the content instead of the
pointer, and others have been worked around be creating fake use
points at block arguments.
If we always simply query the content for escape state and use points,
then we never need to propagate along defer edges. The current code
that propagates escape state along defer edges in one direction is
simply incorrect from the perspective of alias analysis.
One very attractive solution is to merge nodes eagerly without
creating any defer edges, but that would be a much more radical change
even than what I've done here. It would also pose some new issues: how
to resolve the current "node types" when merging and how to deal with
missing content nodes.
This solution of applying escape state to content nodes solves all
these problems without too radical of a change at the expense of
eagerly creating content nodes. (The potential graph memory usage is
not really an issue because it's possible to drastically shrink the
size of the graph anyway in a future commit--I've been able to fit a
node within one cache line). This solution nicely preserves graph
structure which makes it easy to debug and relate to the IR.
Eagerly creating content nodes also solves the missing content node
problem. For example, when querying canEscapeTo, we need to know
whether to look at the escape state for just the pointer value itself,
or also for its content. It may be possible the its content node is
actually part of the same object at the IR level. If the content node
is missing, then we don't know if the object's interior address is not
recognizable/representable or whether we simply never saw an access to
the interior address. We can't simply look at whether the current IR
value happens to be a reference, because that doesn't tell us whether
the graph node may have been merged with a non-reference node or even
with it's own content node. To be correct in general, this query would
need to be extremely conservative. However, if content nodes are
always created for references, then we only need to query the escape
state of a pointer's content node. The content node's flag tells us if
it's an interior node, in which case it will always point to another
content node which also needs to be queried.
EscapeAnalysis: Do not create defer edges for block arguemnts.
That appears to have been a partial workaround for the real
problem that usepoints need to be propagated across the entire
defer web. This is now solved by considering use points on the
reference node's content, not the reference node itself.
EscapeAnalysis: rewrite canEscapeToUsePoint.
Correctness: do not make any unenforced assumptions about how the
connection graph is built (I don't think the previous assumption about
the structure of the graph node mapped to a reference-type value would
always hold if content nodes can be arbitrarily merged). Only make one
assumption about the client code: the access being checked must be to
some address within the provided value, not another object indirectly
reachable from that value.
Optimization: Allow escape analysis to prove that an addressable
object does not escape even when one of its reference-type fields
escapes.
EscapeAnalysis: Add and update tests.
Fix tests for changes to EscapeAnalysis output