This allows for me to do a couple of things improving quality/correctness/ease of use:
1. I reimplemented InstMod's RAUW and RAUW/erase helpers on top of
setUseValue/deleteInst. Beyond allowing the caller to specify less things, we
gain an orthogonality preventing bugs like overriding erase/RAUW but not
overriding erase or having the erase used in erase/RAUW act differently than
the erase for deleteInst.
2. There were a bunch of places using InstModCallback that also were setting
uses without having the ability for InstModCallbacks perform it (since it
only supported RAUW). This is an anti-pattern and could cause subtle bugs to
be introduced by appropriate state in the caller not being updated.
NOTE: The stdlib count/capacity propagation code is tested in an end<->end
fashion in a separate Swift test. Once I flip the switch, that test will run.
The code is pretty simple, so I feel relatively confident with it.
These are always safe in OSSA since what we are doing here is hoisting the
ref_to_raw_pointer up the def-use chain without deleting any instructions unless
we know that they do not have any uses (in a strict sense so destroy_value is
considered a use). E.x.:
```
%0 = ...
%1 = unchecked_ref_cast %0
%2 = ref_to_raw_pointer %1
```
->
```
%0 = ...
%1 = unchecked_ref_cast %0
%2 = ref_to_raw_pointer %0
```
Notice, how we are actually not changing %1 at all. Instead we are just moving
an instantaneous use earlier. One thing that is important to realize is that
this /does/ cause us to need to put the ref_to_raw_pointer at the insert
location of %0 since %0's lifetime ends at the unchecked_ref_cast if the value
is owned.
NOTE: I also identified the tests from sil_combine.sil that had to do with these
simplifications and extracted them into sil_combine_casts.sil and did the
ossa/non-ossa tests side by side. I am trying to fix up the SILCombine tests as
I update stuff, so if I find opportunities to move tests into a more descriptive
sub-file, I am going to do so.
As an aside, to make it easier to transition SILCombine away from using a
central builder, I added a withBuilder method that creates a new SILBuilder at a
requested insertPt and uses the same context as the main builder of
SILCombine. It also through the usage of auto makes really concise pieces of
code. Today to do this just using builder, we would do:
```
SILBuilderWithScope builder(insertPt, Builder);
builder.createInst1(insertPt->getLoc(), ...);
builder.createInst2(insertPt->getLoc(), ...);
builder.createInst3(insertPt->getLoc(), ...);
auto *finalValue = builder.createInst4(insertPt->getLoc(), ...);
```
Thats a lot of typing and wastes a really commonly used temp name (builder) in
the local scope! Instead, using this API, one can write:
auto *finalValue = withBuilder(insertPt, [&](auto &b, auto l) {
b.createInst1(l, ...);
b.createInst2(l, ...);
b.createInst3(l, ...);
return b.createInst4(l, ...);
});
There is significantly less to type and auto handles the types for us. The
withBuilder construct is just syntactic since we always inline it.
This is a generic API that when ownership is enabled allows one to replace all
uses of a value with a value with a differing ownership by transforming/lifetime
extending as appropriate.
This API supports all pairings of ownership /except/ replacing a value with
OwnershipKind::None with a value without OwnershipKind::None. This is a more
complex optimization that we do not support today. As a result, we include on
our state struct a helper routine that callers can use to know if the two values
that they want to process can be handled by the algorithm.
My moticiation is to use this to to update InstSimplify and SILCombiner in a
less bug prone way rather than just turn stuff off.
Noting that this transformation inserts ownership instructions, I have made sure
to test this API in two ways:
1. With Mandatory Combiner alone (to make sure it works period).
2. With Mandatory Combiner + Semantic ARC Opts to make sure that we can
eliminate the extra ownership instructions it inserts.
As one can see from the tests, the optimizer today is able to handle all of
these transforms except one conditional case where I need to eliminate a dead
phi arg. I have a separate branch that hits that today but I have exposed unsafe
behavior in ClosureLifetimeFixup that I need to fix first before I can land
that. I don't want that to stop this PR since I think the current low level ARC
optimizer may be able to help me here since this is a simple transform it does
all of the time.
This works around an issue where using an apply with an unsubstituted
substitution map causes issues in downstream optimizations.
```
%9 = alloc_stack $@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto
copy_addr %8 to [initialization] %9 : $*@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto
%11 = witness_method $ConformerClass, #NonClassProto.myVariable!getter : <Self where Self : NonClassProto> (Self) -> () -> SomeValue :
$@convention(witness_method: NonClassProto) <τ_0_0 where τ_0_0 : NonClassProto> (@in_guaranteed τ_0_0) -> SomeValue
apply %11<@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto>(%9) : $@convention(witness_method: NonClassProto) <τ_0_0 where τ_0_0 : NonClassProto> (@in_guaranteed τ_0_0) -> SomeValue
```
The problem arise when the devirtualizer replace
`witness_method $ConformerClass, #NonClassProto.myVariable!getter` with the
underlying implementation. That implementation for better or worse is further
constrained to `Self : ConformerClass` and applying an opened existential
which is not class constraint is a recipe for disaster. The proper
solution would probably be for the devirtualizer to insert the cast if necessary
and update the substitution list.
That fix will be left for another day though.
rdar://70582785
Optimize the unconditional_checked_cast_addr in this pattern:
%box = alloc_existential_box $Error, $ConcreteError
%a = project_existential_box $ConcreteError in %b : $Error
store %value to %a : $*ConcreteError
%err = alloc_stack $Error
store %box to %err : $*Error
%dest = alloc_stack $ConcreteError
unconditional_checked_cast_addr Error in %err : $*Error to ConcreteError in %dest : $*ConcreteError
to:
...
retain_value %value : $ConcreteError
destroy_addr %err : $*Error
store %value to %dest $*ConcreteError
This lets the alloc_existential_box become dead and it can be removed in following optimizations.
The same optimization is also done for conditional_checked_cast_addr.
There is also an implication for debugging:
Each "throw" in the code calls the runtime function swift_willThrow. The function is used by the debugger to set a breakpoint and also add hooks.
This optimization can completely eliminate a "throw", including the runtime call.
So, with optimized code, the user might not see the program to break at a throw, whereas in the source code it is actually throwing.
On the other hand, eliminating the existential box is a significant performance win and we don't guarantee any debugging behavior for optimized code anyway. So I think this is a reasonable trade-off.
I added an option "-Xllvm -keep-will-throw-call" to keep the runtime call which can be used if someone want's to reliably break on "throw" in optimized builds.
rdar://problem/66055678
This reinstates commit d7d829c059 with a fix for C tail-allocated arrays.
Replace a call of the getter of AnyKeyPath._storedInlineOffset with a "constant" offset, in case of a keypath literal.
"Constant" offset means a series of struct_element_addr and tuple_element_addr instructions with a 0-pointer as base address.
These instructions can then be lowered to "real" constants in IRGen for concrete types, or to metatype offset lookups for generic or resilient types.
Replace:
%kp = keypath ...
%offset = apply %_storedInlineOffset_method(%kp)
with:
%zero = integer_literal $Builtin.Word, 0
%null_ptr = unchecked_trivial_bit_cast %zero to $Builtin.RawPointer
%null_addr = pointer_to_address %null_ptr
%projected_addr = struct_element_addr %null_addr
... // other address projections
%offset_ptr = address_to_pointer %projected_addr
%offset_builtin_int = unchecked_trivial_bit_cast %offset_ptr
%offset_int = struct $Int (%offset_builtin_int)
%offset = enum $Optional<Int>, #Optional.some!enumelt, %offset_int
rdar://problem/53309403
Replace a call of the getter of AnyKeyPath._storedInlineOffset with a "constant" offset, in case of a keypath literal.
"Constant" offset means a series of struct_element_addr and tuple_element_addr instructions with a 0-pointer as base address.
These instructions can then be lowered to "real" constants in IRGen for concrete types, or to metatype offset lookups for generic or resilient types.
Replace:
%kp = keypath ...
%offset = apply %_storedInlineOffset_method(%kp)
with:
%zero = integer_literal $Builtin.Word, 0
%null_ptr = unchecked_trivial_bit_cast %zero to $Builtin.RawPointer
%null_addr = pointer_to_address %null_ptr
%projected_addr = struct_element_addr %null_addr
... // other address projections
%offset_ptr = address_to_pointer %projected_addr
%offset_builtin_int = unchecked_trivial_bit_cast %offset_ptr
%offset_int = struct $Int (%offset_builtin_int)
%offset = enum $Optional<Int>, #Optional.some!enumelt, %offset_int
rdar://problem/53309403
Replaces an alloc_stack of an enum by an alloc_stack of the payload if only one enum case (with payload) is stored to that location.
For example:
%loc = alloc_stack $Optional<T>
%payload = init_enum_data_addr %loc
store %value to %payload
...
%take_addr = unchecked_take_enum_data_addr %loc
%l = load %take_addr
is transformed to
%loc = alloc_stack $T
store %value to %loc
...
%l = load %loc
https://bugs.swift.org/browse/SR-12710
I am going to use this in mandatory combine, and it seems like a generally
useful transformation.
I also updated the routine to construct its own SILBuilder that injects a user
passed in SILBuilderContext eliminating the bad pattern of passing in
SILBuilders.
This should be an NFC change.
Changes:
* Allow optimizing partial_apply capturing opened existential: we didn't do this originally because it was complicated to insert the required alloc/dealloc_stack instructions at the right places. Now we have the StackNesting utility, which makes this easier.
* Support indirect-in parameters. Not super important, but why not? It's also easy to do with the StackNesting utility.
* Share code between dead closure elimination and the apply(partial_apply) optimization. It's a bit of refactoring and allowed to eliminate some code which is not used anymore.
* Fix an ownership problem: We inserted copies of partial_apply arguments _after_ the partial_apply (which consumes the arguments).
* When replacing an apply(partial_apply) -> apply and the partial_apply becomes dead, avoid inserting copies of the arguments twice.
These changes don't have any immediate effect on our current benchmarks, but will allow eliminating curry thunks for existentials.
The XXOptUtils.h convention is already established and parallels
the SIL/XXUtils convention.
New:
- InstOptUtils.h
- CFGOptUtils.h
- BasicBlockOptUtils.h
- ValueLifetime.h
Removed:
- Local.h
- Two conflicting CFG.h files
This reorganization is helpful before I introduce more
utilities for block cloning similar to SinkAddressProjections.
Move the control flow utilies out of Local.h, which was an
unreadable, unprincipled mess. Rename it to InstOptUtils.h, and
confine it to small APIs for working with individual instructions.
These are the optimizer's additions to /SIL/InstUtils.h.
Rename CFG.h to CFGOptUtils.h and remove the one in /Analysis. Now
there is only SIL/CFG.h, resolving the naming conflict within the
swift project (this has always been a problem for source tools). Limit
this header to low-level APIs for working with branches and CFG edges.
Add BasicBlockOptUtils.h for block level transforms (it makes me sad
that I can't use BBOptUtils.h, but SIL already has
BasicBlockUtils.h). These are larger APIs for cloning or removing
whole blocks.
In the previous commit, various methods for adding, replacing, and
removing instructions were duplicate from SILCombiner into
SILInstructionWorklist. Here, SILCombiner is modified to call through
to the methods which were added to SILInstructionWorklist.
- Replaced usage of raw map and vector with the type that wraps the
combination (BlotSetVector); that provided a significant deduplication
since a sizeable portion of the worklist's implementation was in
vector and map management now provided by the BlotSetVector.
- Templated over the type of map and vector used by the blot set vector.
- Added SmallSILInstructionWorklist where the map and vector are
specified to be SmallDenseMap and SmallVector respectively.
- Replaced usages of bare ValueBase with usages of SILValue.
- Renamed zap to resetChecked.
Added a bit of functionality to BlotSetVector, specifically to support
SILInstructionWorklist:
- Made insert return not just the index that the potentially-inserted
item is at on return but additionally whether an insertion occurred,
matching the behavior of llvm::DenseMap::insert.
- Added a method to reserve capacity in the backing vector and map:
BlotSetVector::reserve.
- Added a method to free extra storage used by the backing vector and
map: BlotSetVector::clear.
- Modified SmallBlotSetVector's template parameters so that only a value
and size can be specified--that type will always use SmallVector and
SmallDenseMap for its superclass' VectorT and MapT template
parameters.
- Updated variable names.
In the previous commit, SILInstructionWorklist was added as a verbatim
extraction (modulo some minor style tweaks) of SILCombineWorklist.
Here, SILCombine is moved over to using that renamed type.
Returns `true` if `T.Type` is known to refer to a concrete type. The
implementation allows for the optimizer to specialize this at -O and
eliminate conditional code.
Includes `Swift._isConcrete<T>(T.Type) -> Bool` wrapper function.
If the keypath argument of a keypath access function is a keypath literal instruction, generate the projection inline and remove the access function.
For example, replaces (simplified SIL):
%kp = keypath ... stored_property #Foo.bar
apply %keypath_runtime_function(%root_object, %kp, %addr)
with:
%addr = struct_element_addr %root_object, #Foo.bar
load/store %addr
Currently this only handles stored property patterns.
rdar://problem/36244734
This is the complement to load canonicalization. Although store
canonicalization is not required before diagnostics, it should be
defined in the same utility.
NOTE: I changed all places that the CastOptimizer is created to just pass in
nullptr for now so this is NFC.
----
Right now the interface of the CastOptimizer is muddled and confused. Sometimes
it is returning a value that should be used by the caller, other times it is
returning an instruction that is meant to be reprocessed by the caller.
This series of patches is attempting to clean this up by switching to the
following model:
1. If we are optimizing a cast of a value, we return a SILValue. If the cast
fails, we return an empty SILValue().
2. If we are optimizing a cast of an address, we return a boolean value to show
success/failure and require the user to use the SILBuilderContext to get the
cast if they need to.
Generalizes the ConcreteExistentialInfo abstraction so it can be used
both by the ExistentialSpecializer and SILCombine, allowing redundant
code in ExistentialSpecializer.cpp to be deleted.
Splits OpenedArchetypeInfo from ConcreteExistentialInfo. Adds a
ConcreteOpenedArchetypeInfo convenience wrapper around them both, for
use wherever we were originally using ConcreteExistentialInfo.
Splits getAddressOfStackInit into getStackInitInst, This is cleaner and
allows both the ExistentialSpecializer and SILCombine to handle more
interesting cases in the future, like unconditional_checked_cast.
Creates utilities, initializeSubstitutionMap, and
initializeConcreteTypeDef to simplify an generalize
ConcreteExistentialInfo.
While rewriting ExistentialSpecializer to use the new
abstraction, I fixed a latent bug in which is was using a SIL
argument index as a function type parameter index (this would
have broken up if/when we decide to enable calls with indirect
results).
We've been running doxygen with the autobrief option for a couple of
years now. This makes the \brief markers into our comments
redundant. Since they are a visual distraction and we don't want to
encourage more \brief markers in new code either, this patch removes
them all.
Patch produced by
for i in $(git grep -l '\\brief'); do perl -pi -e 's/\\brief //g' $i & done
I am going to add the code in a bit that does the notifications. I tried to pass
down the builder instead of the pass manager. I also tried not to change the
formatting.
rdar://42301529
Fixes <rdar://40555427> [SR-7773]:
SILCombiner::propagateConcreteTypeOfInitExistential fails to full propagate type
substitutions.
Fixes <rdar://problem/40923849>
SILCombiner::propagateConcreteTypeOfInitExistential crashes on protocol
compositions.
This rewrite fixes several fundamental bugs in the SILCombiner optimization that
propagates concrete types. In particular, the pass needs to handle:
- Arguments of callee Self type in non-self position.
- Indirect and direct return values of Self type.
- Types that indirectly depend on Self within callee function signature.
- Protocol composition existentials.
- All of the above need to work for protocol extensions as well as witness methods.
- For protocol extensions, conformance lookup should be based on the existential's conformance list.
Additionally, the optimization should not depend on a SILFunction's DeclContext,
which is not serialized. (In fact, we should prevent SIL passes from using
DeclContext). Furthermore, the code needs to be expressed in a way that one can
reason about correctness and invariants.
The root cause of these bugs is that SIL passes are written based on untested
assumptions of Swift type system. A SIL pass needs to handle all verifiable SIL
input because passes need to be composable. Bail-out logic can be added to
simplify the design; however, _the bail-out logic itself cannot make any
assumptions about the language or type system_ that aren't clearly and
explicitly enforced in the SIL verifier. This is a common mistake and major
source of bugs.
I created as many unit tests as I reasonably could to prevent this code from
regressing. Creating enough unit tests to cover all corner cases that were
broken in the original code would be intractable. But the code has been
simplified such that many corner cases disappear.
This opens up some oportunity for generalizing the optimization and eliminating
special cases. However, I want this PR to be limited to fixing correctness
issues only. In the long term, it would be preferable to replace this
optimization entirely with a much more powerful general type propagation pass.
For example:
%0 = string_literal "abc"
%1 = integer_literal 0x8000000000000000
%2 = builtin "stringObjectOr_Int64" (%0, %1)
%3 = integer_literal 0x4000000000000000
%4 = builtin "and_Int64" (%2, %3)
In this case we know that %4 is 0.
Certain patterns of directly applied partial_apply's were not being
inlined. This can happen when a closure is defined in the
implementation of a generic witness method. I noticed this issue while
debugging SR-6254: "Fix (or explain) strange ways to make code >3
times faster/slower".
After the witness method is devirtualization and specialized, the
closure application looks like this:
%pa = partial_apply [callee_guaranteed] %fn() : $@convention(thin) (@in Int) -> @out Int
%cvt = convert_escape_to_noescape %pa
apply %cvt(...)
SILCombine already removes the partial_apply and convert_escape_to_noescape generating:
%thick = thin_to_thick %fn
apply %thick(...)
However, surprisingly, neither the inliner nor SILCombine can handle this.
I cleaned up the code in SILCombine's apply visitor that handles
thin_to_thick and generalized it to handle this and other patterns.
I also added a restriction to this code so that it doesn't break SIL
ownership in the future, as I discussed with Arnold.
Certain patterns of directly applied partial_apply's were not being
inlined. This can happen when a closure is defined in the
implementation of a generic witness method. I noticed this issue while
debugging SR-6254: "Fix (or explain) strange ways to make code >3
times faster/slower".
After the witness method is devirtualization and specialized, the
closure application looks like this:
%pa = partial_apply [callee_guaranteed] %fn() : $@convention(thin) (@in Int) -> @out Int
%cvt = convert_escape_to_noescape %pa
apply %cvt(...)
SILCombine already removes the partial_apply and convert_escape_to_noescape generating:
%thick = thin_to_thick %fn
apply %thick(...)
However, surprisingly, neither the inliner nor SILCombine can handle this.
I cleaned up the code in SILCombine's apply visitor that handles
thin_to_thick and generalized it to handle this and other patterns.
I also added a restriction to this code so that it doesn't break SIL
ownership in the future, as I discussed with Arnold.
Local.cpp was ~3k lines of which 1.5k (i.e. 1/2) was the cast optimizer. This
commit extracts the cast optimizer into its own .cpp and .h file. It is large
enough to stand on its own and allows for Local.cpp to return to being a small
group of helper functions.
I am making some changes in this area due to the change in certain function
conventions caused by the +0-normal-arg work. I am just trying to leave the area
a little cleaner than before.
* Reduce array abstraction on apple platforms dealing with literals
Part of the ongoing quest to reduce swift array literal abstraction
penalties: make the SIL optimizer able to eliminate bridging overhead
when dealing with array literals.
Introduce a new classify_bridge_object SIL instruction to handle the
logic of extracting platform specific bits from a Builtin.BridgeObject
value that indicate whether it contains a ObjC tagged pointer object,
or a normal ObjC object. This allows the SIL optimizer to eliminate
these, which allows constant folding a ton of code. On the example
added to test/SILOptimizer/static_arrays.swift, this results in 4x
less SIL code, and also leads to a lot more commonality between linux
and apple platform codegen when passing an array literal.
This also introduces a couple of SIL combines for patterns that occur
in the array literal passing case.
