Reimplement the simplification in swift and add a new transformation:
```
%1 = unchecked_addr_cast %0 : $*Builtin.FixedArray<N, Element> to $*Element
```
->
```
%1 = vector_base_addr %0 : $*Builtin.FixedArray<N, Element>
```
Which consists of
* removing redundant `address_to_pointer`-`pointer_to_address` pairs
* optimize `index_raw_pointer` of a manually computed stride to `index_addr`
* remove or increase the alignment based on a "assumeAlignment" builtin
This is a big code cleanup but also has some functional differences for the `address_to_pointer`-`pointer_to_address` pair removal:
* It's not done if the resulting SIL would result in a (detectable) use-after-dealloc_stack memory lifetime failure.
* It's not done if `copy_value`s must be inserted or borrow-scopes must be extended to comply with ownership rules (this was the task of the OwnershipRAUWHelper).
Inserting copies is bad anyway.
Extending borrow-scopes would only be required if the original lifetime of the pointer extends a borrow scope - which shouldn't happen in save code. Therefore this is a very rare case which is not worth handling.
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
`getValue` -> `value`
`getValueOr` -> `value_or`
`hasValue` -> `has_value`
`map` -> `transform`
The old API will be deprecated in the rebranch.
To avoid merge conflicts, use the new API already in the main branch.
rdar://102362022
strong_retain/strong_release with an optional reference as operand are rejected by the verifier and are not supported by IRGen.
SILCombine created such retains/releases when optimizing ref_cast instructions.
This fixes a compiler crash.
rdar://74146617
Instead make `findJointPostDominatingSet` a stand-alone function.
There is no need to keep the temporary SmallVector alive across multiple calls of findJointPostDominatingSet for the purpose of re-using malloc'ed memory. The worklist usually contains way less elements than its small size.
Some notes:
1. I moved the identity round-trip case to InstSimplify since that is where
optimizations like that are.
2. I did not update in this commit the code that eliminates convert_function
when it is only destroyed. In a subsequent commit I am going to implement
that in a general way and apply it to all forwarding instructions.
3. I implemented eliminating convert_function with ownership only uses in a
utility so that I can reuse it for other similar optimizations in SILCombine.
I am trying to be more careful about this rather than letting someone make this
mistake in the future. I also added some comments and a DeadEndBlocks instance
to TempRValueElimination so that it gets full simplifications in OSSA.
Btw for those curious, the reason why this optimization is important is that the
stdlib often times performs reinterpret casts using address to pointer, pointer
to address to perform the case. So we need to be able to reliably eliminate
these. Since the underlying base object's liveness is already what provides the
liveness to the underlying address, our approach of treating this as a lifetime
extension requiring pattern makes sense since we are adding information into the
IR by canonicalizing these escapes to be normal address operations.
NOTE: In this commit, we only handle the identity transform. In a subsequent
commit, I am going to hit the non-identity transform that is handled by
SILCombine.
Also, I added some specific more general OSSA RAUW tests for this case to
ossa_rauw_tests using this functionality in a more exhaustive way that would be
in appropriate in sil_combine_ossa.sil (since I am stress testing this specific
piece of code).
In OSSA, we enforce that addresses from interior pointer instructions are scoped
within a borrow scope. This means that it is invalid to use such an address
outside of its parent borrow scope and as a result one can not just RAUW an
address value by a dominating address value since the latter may be invalid at
the former. I foresee that I am going to have to solve this problem and so I
decided to write this API to handle the vast majority of cases.
The way this API works is that it:
1. Computes an access path with base for the new value. If we do not have a base
value and a valid access path with root, we bail.
2. Then we check if our base value is the result of an interior pointer
instruction. If it isn't, we are immediately done and can RAUW without further
delay.
3. If we do have an interior pointer instruction, we see if the immediate
guaranteed value we projected from has a single borrow introducer value. If not,
we bail. I think this is reasonable since with time, all guaranteed values will
always only have a single borrow introducing value (once struct, tuple,
destructure_struct, destructure_tuple become reborrows).
4. Then we gather up all inner uses of our access path. If for some reason that
fails, we bail.
5. Then we see if all of those uses are within our borrow scope. If so, we can
RAUW without any further worry.
6. Otherwise, we perform a copy+borrow of our interior pointer's operand value
at the interior pointer, create a copy of the interior pointer instruction upon
this new borrow and then RAUW oldValue with that instead. By construction all
uses of oldValue will be within this new interior pointer scope.
The reason why I am doing this is that I am building up more utilities based on
passing around this struct of context that do not want it for RAUWing
purposes. So it makes sense on a helper (OwnershipRAUWHelper) that composes with
its state.
Just a refactor, should be NFC.
Currently all of these places in the code base perform simplifyInstruction and
then a replaceAllSimplifiedUsesAndErase(...). This is a bad pattern since:
1. simplifyInstruction assumes its result will be passed to
replaceAllSimplifiedUsesAndErase. So by leaving these as separate things, we
allow for users to pointlessly make this mistake.
2. I am going to implement in a subsequent commit a utility that lifetime
extends interior pointer bases when replacing an address with an interior
pointer derived address. To do this efficiently, I want to reuse state I
compute during simplifyInstruction during the actual RAUW meaning that if the
two operations are split, that is difficult without extending the API. So by
removing this, I can make the transform and eliminate mistakes at the same
time.
This is a low level API already being used in multiple places besides
InstSimplify (e.x.: Utils/OwnershipOptUtils), so it makes sense to move it into
InstOptUtil.
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.
Specifically before this PR, if a caller did not customize a specific callback
of InstModCallbacks, we would store a static default std::function into
InstModCallbacks. This means that we always would have an indirect jump. That is
unfortunate since this code is often called in loops.
In this PR, I eliminate this problem by:
1. I made all of the actual callback std::function in InstModCallback private
and gave them a "Func" postfix (e.x.: deleteInst -> deleteInstFunc).
2. I created public methods with the old callback names to actually call the
callbacks. This ensured that as long as we are not escaping callbacks from
InstModCallback, this PR would not result in the need for any source changes
since we are changing a call of a std::function field to a call to a method.
3. I changed all of the places that were escaping inst mod's callbacks to take
an InstModCallback. We shouldn't be doing that anyway.
4. I changed the default value of each callback in InstModCallbacks to be a
nullptr and changed the public helper methods to check if a callback is
null. If the callback is not null, it is called, otherwise the getter falls
back to an inline default implementation of the operation.
All together this means that the cost of a plain InstModCallback is reduced and
one pays an indirect function cost price as one customizes it further which is
better scalability.
P.S. as a little extra thing, I added a madeChange field onto the
InstModCallback. Now that we have the helpers calling the callbacks, I can
easily insert instrumentation like this, allowing for users to pass in
InstModCallback and see if anything was RAUWed without needing to specify a
callback.
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.
If the only use of an upcast, unchecked_ref_cast or end_cow_mutation is a destroy/release, just destroy the operand and remove the cast/end_cow_mutation.
Tuple, Struct, and Enum MetatypesTypes have a single value. In this
case, replace metatype instructions with arguments of the same
MetatypeType to enable downstream CSE and SILCombines.
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.
The SIL generation for this builtin also changes: instead of generating the cond_fail instructions upfront, let the optimizer generate it, if the operand is a static string literal.
In worst case, if the second operand is not a static string literal, the Builtin.condfail is lowered at the end of the optimization pipeline with a default message: "unknown program error".
The SIL generation for this builtin also changes: instead of generating the cond_fail instructions upfront, let the optimizer generate it, if the operand is a static string literal.
In worst case, if the second operand is not a static string literal, the Builtin.condfail is lowered at the end of the optimization pipeline with a default message: "unknown program error".
This will make the forthcoming CanonicalizeInstruction interface more
clear.
This is generally the better approach to utilities that mutate the
instruction stream. It avoids the temptation to assume that only a
single instruction will be deleted or that only instructions before
the current iterator will be deleted. This often happens to work but
eventually fails in the presense of debug and end-of-scope
instructions.
A function returning an iterator has a more clear contract than one
accepting some iterator reference of unknown
providence. Unfortunately, it doesn't work at the lowest level of
utilities, such as recursivelyDeleteTriviallyDeadInstructions, where
we want to handle instruction batches.
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
* Remove apparently obsolete builtin functions.
- Remove s_to_u_checked_conversion and u_to_s_checked_conversion functions from builtin AST parsing, SIL/IR generation and from SIL optimisations.
* Remove apparently obsolete builtin functions - unit tests.
- Remove unit tests for SIL transformations relating to s_to_u_checked_conversion and u_to_s_checked_conversion builtin functions.
* Remove apparently obsolete builtin functions.
- Remove s_to_u_checked_conversion and u_to_s_checked_conversion functions from builtin AST parsing, SIL/IR generation and from SIL optimisations.
* Remove apparently obsolete builtin functions - unit tests.
- Remove unit tests for SIL transformations relating to s_to_u_checked_conversion and u_to_s_checked_conversion builtin functions.
Replace `NameOfType foo = dyn_cast<NameOfType>(bar)` with DRY version `auto foo = dyn_cast<NameOfType>(bar)`.
The DRY auto version is by far the dominant form already used in the repo, so this PR merely brings the exceptional cases (redundant repetition form) in line with the dominant form (auto form).
See the [C++ Core Guidelines](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es11-use-auto-to-avoid-redundant-repetition-of-type-names) for a general discussion on why to use `auto` to avoid redundant repetition of type names.
This was already done for getSuccessorBlocks() to distinguish getting successor
blocks from getting the full list of SILSuccessors via getSuccessors(). This
commit just makes all of the successor/predecessor code follow that naming
convention.
Some examples:
getSingleSuccessor() => getSingleSuccessorBlock().
isSuccessor() => isSuccessorBlock().
getPreds() => getPredecessorBlocks().
Really, IMO, we should consider renaming SILSuccessor to a more verbose name so
that it is clear that it is more of an internal detail of SILBasicBlock's
implementation rather than something that one should consider as apart of one's
mental model of the IR when one really wants to be thinking about predecessor
and successor blocks. But that is not what this commit is trying to change, it
is just trying to eliminate a bit of technical debt by making the naming
conventions here consistent.
Strict aliasing only applies to memory operations that use strict
addresses. The optimizer needs to be aware of this flag. Uses of raw
addresses should not have their address substituted with a strict
address.
Also add Builtin.LoadRaw which will be used by raw pointer loads.
