evaluable calls. The fix makes the check more conservative and
assumes that calls with generic arguments are not dead, as generic
functions with arbitrary side-effects can be invoked through them.
Also, add a few helper functions to the InstructionDeleter utility
that will enable deleting an instruction along with its users.
Consistently handle base and derived pointers.
Consistently avoid creating nodes for undef, which breaks
verification.
Be more precise about creating the node based on the derived pointer
type.
Remove a few extraneous helpers.
Fixes <rdar://58445744> swiftc assert during EscapeAnalysis
verification: (EA->isPointer(Nd->mappedValue)), function verify
Don't merge node properties until after node merging begins, so
internal verification can run right before each merge.
Rework ConnectionGraph::mergeFrom. Remove an extra loop. Defer
mergeAllScheduledNodes until all the source graph's mapped nodes are
added so that the graph is always structurally valid before a
merge. This is also necessary to avoid EscapeAnalysis
assert: (!To->mergeTo), in setPointsToEdge.
Enable -escapes-internal-verify to all tests in escape_analysis.sil.
Add hand-reduced unit tests in escape_analysis_reduced.sil.
Use the new EscapeAnalysis infrastructure to make ARC code motion and
ARC sequence opts much more powerful and fix a latent bug in
AliasAnalysis.
Adds a new API `EscapeAnalysis::mayReleaseContent()`. This replaces
all uses if `EscapeAnalysis::canEscapeValueTo()`, which affects
`AliasAnalysis::can[Apply|Builtin]DecrementRefCount()`.
Also rewrite `AliasAnalysis::mayValueReleaseInterferWithInstruction` to
directly use `EscapeAnalysis::mayReleaseContent`.
The new implementation in `EscapeAnalysis::mayReleaseContent()`
generalizes the logic to handle more cases while avoiding an incorrect
assumption in the prior code. In particular, it adds support for
disambiguating local references from accessed addresses. This helps
handle cases in which inlining was defeating ARC optimization. The
incorrect assumption was that a non-escaping address is never
reachable via a reference. However, if a reference does not escape,
then an address into its object also does not escape.
The bug in `AliasAnalysis::mayValueReleaseInterfereWithInstruction()`
appears not to have broken anything yet because it is always called by
`AliasAnalysis::mayHaveSymmetricInteference()`, which later checks
whether the accessed address may alias with the released reference
using a separate query, `EscapeAnalysis::canPointToSameMemory()`. This
happens to work because an address into memory that is directly
released when destroying a reference necesasarilly points to the same
memory object. For this reason, I couldn't figure out a simple way to
hand-code SIL tests to expose this bug.
The changes in diff order:
Replace EscapeAnalysis `canEscapeToValue` with `mayReleaseContent` to
make the semantics clear. It queries: "Can the given reference release
the content pointed to the given address".
Change `AliasAnalysis::canApplyDecrementRefCount` to use
`mayReleaseContent` instead if 'canEscapeToValue'.
Change `AliasAnalysis::mayValueReleaseInterferWithInstruction`: after
getting the memory address accessed by the instruction, simply call
`EscapeAnalysis::mayReleaseContent`, which now implements all the
logic. This avoids the bad assumption made by AliasAnalysis.
Handle two cases in mayReleaseContent: non-escaping instruction
addresses and non-escaping referenecs. Fix the non-escaping address
case by following all content nodes to determine whether the address
is reachable from the released reference. Introduce a new optimization
for the case in which the reference being released is allocated
locally.
The following test case is now optimized in arcsequenceopts.sil:
remove_as_local_object_indirectly_escapes_to_callee. It was trying to
test that ARC optimization was not too aggressive when it removed a
retain/release of a child object whose parent container is still in
use. But the retain/release should be removed. The original example
already over-releases the parent object.
Add new unit tests to late_release_hoisting.sil.
StackPromotion queries EscapeAnalysis. The escape information for the
result of an array semantic was missing because it was an ill-formed
call. This fix introduces a new check to determine whether a call is
well formed so it can be handled consistently everywhere.
Fixes <rdar://problem/58113508>
Interpreter/SDK/objc_fast_enumeration.swift failed on
iphonesimulator-i386
The bug was introduced here:
commit 8b926af49a
Author: Andrew Trick <atrick@apple.com>
Date: Mon Dec 16 16:04:09 2019 -0800
EscapeAnalysis: Add PointerKind and interior/reference flags
The bug can occur when a semantic "array.ininitialized"
call (Array.adoptStorage) at the SIL level has direct "release_value"
instructions that don't use the tuple_extract values corresponding to
the call's returned value. This is part of the tragedy of not
supporting multiple call results. When users of the call's result can
use either the entire tuple (which is a meaningless value), or the
individual tuple extracts, then there's no way to handle calls
uniformly.
The change that broke this was to remove the special handling of
TupleExtract instructions. That special handling was identical to the
way that any normal pointer projection is handled. So, as a
simplification, the new code just expects that case to be handled
naturally as a pointer projection. This case was already guarded by a
check to determine whether the TupleExtract was actually the result of
an array.uninitialized call, in which case the normal handling does
not apply. However, because of the weirdness mentioned above, the
handling of "array.ininitialized" may silently bail out. When that
happens, the TupleExtract gets neither the special handling, nor the
normal handling.
Note that the old special handling of TupleExtract was bizarre and
relied on corresponding weirdness in the code that handles
"array.uninitialized". The new code simply creates a separate graph
node for each result of the call, and creates the edges that exactly
correspond to load and stores on those results. So, rather than
reinstating the previous code, which I can't quite reason about, this
fix creates a single check to determine whether a TupleExtract is
treated as an "array.uninitialized" result or not, and use that check
in both places.
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.
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.
and eliminate dead code. This is meant to be a replacement for the utility:
recursivelyDeleteTriviallyDeadInstructions. The new utility performs more aggresive
dead-code elimination for ownership SIL.
This patch also migrates most non-force-delete uses of
recursivelyDeleteTriviallyDeadInstructions to the new utility.
and migrates one force-delete use of recursivelyDeleteTriviallyDeadInstructions
(in IRGenPrepare) to use the new utility.
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.
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.
We have an optimization in SILCombiner that "inlines" the use of compile-time constant key paths by performing the property access directly instead of calling a runtime function (leading to huge performance gains e.g. for heavy use of @dynamicMemberLookup). However, this optimization previously only supported key paths which solely access stored properties, so computed properties, optional chaining, etc. still had to call a runtime function. This commit generalizes the optimization to support all types of key paths.
It must be consistent, otherwise the specialized function types may not match for calls in functions with different resilience expansions.
Fixes an assertion crash in the generic specializer.
rdar://problem/57844964
Otherwise it can happen that e.g. specialization runs between CrossModuleSerializationSetup and serialization, resulting that an inlinable function references a shared function (which doesn't have a public linkage).
The solution is to move serialization right after CrossModuleSerializationSetup. But only do that if cross-module-optimization is enabled (it would be a disruptive change to move serialization in general).
Create a Predecessor abstraction for readability. Eliminates frequent
occurence of inscrutable getInt() and getPointer() calls.
Create an escapesInsideFunction() API that only uses mapped
values. This prevents bugs where a content node's value would be
mistakenly used to check for escapes.
This is a first version of cross module optimization (CMO).
The basic idea for CMO is to use the existing library evolution compiler features, but in an automated way. A new SIL module pass "annotates" functions and types with @inlinable and @usableFromInline. This results in functions being serialized into the swiftmodule file and thus available for optimizations in client modules.
The annotation is done with a worklist-algorithm, starting from public functions and continuing with entities which are used from already selected functions. A heuristic performs a preselection on which functions to consider - currently just generic functions are selected.
The serializer then writes annotated functions (including function bodies) into the swiftmodule file of the compiled module. Client modules are able to de-serialize such functions from their imported modules and use them for optimiations, like generic specialization.
The optimization is gated by a new compiler option -cross-module-optimization (also available in the swift driver).
By default this option is off. Without turning the option on, this change is (almost) a NFC.
rdar://problem/22591518
This is needed for cross-module-optimization: CMO marks functions as inlinable. If a private or internal method is referenced from such an inlinable function, it must not be eliminated by dead function elimination after serialization (a method is basically an AbstractFunctionDecl).
For SILFunctions we can do this by simply setting the linkage, but for methods we need another mechanism.
These are separate, mostly unrelated passes. Putting them in their own
files makes it easier to read the code, understand how to control the
passes, and makes it possible to independently trace, and debug them.
Avoid dangling pointers in the connection graph when SILValues are
deleted. This didn't show up in normal compilation because the values
were only used for printing the graph. But now we have more assertions
that check for well-formed graph. These could hit the dangling
pointers.
To guarantee this can't happen, establish a strict invariant that the
mappedValue is only used for nodes that exist in the value-to-node
map. Then clear out mappedValue whenever a value is deleted. Add
verification to ensure that each node's mappedValue is valid and catch
dangling pointers quickly.
This completely changes the way that a node values are set and used
which in turn makes it *much* easier to debug the graph and associate
the nodes with the SIL function. For example, it's normal to view or
dump the graph at a key point. When the nodes are printed, the
associated SILValues are now more precise and consistent. However,
it's also normal to call CGNode::dump() from tracing code or within a
debugger. Previously, there was no way to assocate the output of
CGNode::dump() with the printed or displayed connection graph. Now
both forms of output have identical node IDs!
While we're at it, make sure the hasRC flag is always merged
conservatively in a couple more places.
Fixed <rdar://57290845> While running SILFunctionTransform "TempRValueOpt"
This pipeline just runs the Serialize SIL pass. The reason I am adding this is
that currently if one passes -disable-sil-perf-optzns or mess with
-sil-opt-pass-count, one can cause serialization to not occur, making it
difficult to bisect/turn off-on opts.
Fixes a potential real bug in the case that SinkAddressProjections moves
projections without notifying SimplifyCFG of the change. This could
fail to update Analyses (probably won't break anything in practice).
Introduce SILInstruction::isPure. Among other things, this can tell
you if it's safe to duplicate instructions at their
uses. SinkAddressProjections should check this before sinking uses. I
couldn't find a way to expose this as a real bug, but it is a
theoretical bug.
Add the SinkAddressProjections functionality to the BasicBlockCloner
utility. Enable address projection sinking for all BasicBlockCloner
clients (the four different kinds of jump-threading that use it). This
brings the compiler much closer to banning all address phis.
The "bugs" were originally introduced a week ago here:
commit f22371bf0b (fork/fix-address-phi, fix-address-phi)
Author: Andrew Trick <atrick@apple.com>
Date: Tue Sep 17 16:45:51 2019
Add SIL SinkAddressProjections utility to avoid address phis.
Enable this utility during jump-threading in SimplifyCFG.
Ultimately, the SIL verifier should prevent all address-phis and we'll
need to use this utility in a few more places.
Fixes <rdar://problem/55320867> SIL verification failed: Unknown
formal access pattern: storage
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.
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.
