LLVM, as of 77e0e9e17daf0865620abcd41f692ab0642367c4, now builds with
-Wsuggest-override. Let's clean up the swift sources rather than disable
the warning locally.
TLDR: This fixes an ownership verifier assert caused by not placing end_borrows
along paths where an enum is provable to have a trivial case. It only happens if
all non-trivial cases in a switch_enum are "dead end blocks" where the program
will end and we leak objects.
The Problem
-----------
The actual bug here only occurs in cases where we have a switch_enum on an enum
with mixed trivial, non-trivial cases and all of the non-trivial payloaded cases
are "dead end blocks". As an example, lets look at a simple switch_enum over an
optional where the .some case is a dead end block and we leak the Klass object
into program termination:
```
%0 = load [copy] %mem : $Klass
switch_enum %0 : $Optional<Klass>, case #Optional.some: bbDeadEnd, case #Optional.none: bbContinue
bbDeadEnd(%0a : @owned $Klass): // %0 is leaked into program end!
unreachable
bbContinue:
... // program continue.
```
In this case, if we were only looking at final destroying uses, we would pass a
def without any uses to the ValueLifetimeChecker causing us to not have a
frontier at all causing us to not insert any end_borrows, yielding:
```
%0 = load_borrow %mem : $Klass
switch_enum %0 : $Optional<Klass>, case #Optional.some: bbDeadEnd, case #Optional.none: bbContinue
bbDeadEnd(%0a : @guaranteed $Klass): // %0 is leaked into program end and
// doesnt need an end_borrow!
unreachable
bbContinue:
... // program continue... we need an end_borrow here though!
```
This then trips the ownership verifier since switch_enum is a transforming
terminator that acts like a forwarding instruction implying we need an
end_borrow on the base value along all non-dead end paths through the program.
Importantly this is not actually a leak of a value or unsafe behavior since the
only time that we enter into unsafe territory is along paths where the enum was
actually trivial. So the load_borrow is actually just loaded the trivial enum
value.
The Fix
-------
In order to work around this, I realized that the right solution is to also
include the forwarding consuming uses (in this case the switch_enum use) when
determining the lifetime and that this solves the problem.
That being said, after I made that change, I noticed that I needed to remove my
previous manner of computing the insertion point to use for arguments when
finding the lifetime using ValueLifetimeAnalysis. Previously since I was using
only the destroying uses I knew that the destroy_value could not be the first
instruction in the block of my argument since I handled that case individually
before using the ValueLifetimeAnalysis. That invariant is no longer true as can
be seen in the case above if %0 was from a SILArgument itself instead of a load
[copy] and we were converting that argument to be a guaranteed argument.
To fix this, I taught ValueLifetimeAnalysis how to handle defs from
Arguments. The key thing is I noticed while reading the code that the analysis
only generally cared about the instruction's parent block. Beyond that, the def
being from an instruction was only needed to determine if a user is earlier in
the same block as the def instruction. Those concerns not apply to SILArgument
which dominate all instructions in the same block, so in this patch, we just
skip those conditional checks when we have a SILArgument. The rest of the code
that uses the parent block is the same for both SILArgument/SILInstructions.
rdar://65244617
Specifically:
1. I made methods, variables camelCase.
2. I expanded out variable names (e.x.: bb -> block, predBB -> predBlocks, U -> wrappedUse).
3. I changed typedef -> using.
4. I changed a few c style for loops into for each loops using llvm::enumerate.
NOTE: I left the parts needed for syncing to LLVM in the old style since LLVM
needs these to exist for CRTP to work correctly for the SILSSAUpdater.
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
Since libDemangling is included in the Swift standard library,
ODR violations can occur on platforms that allow statically
linking stdlib if Swift code is linked with other compiler
libraries that also transitively pull in libDemangling, and if
the stdlib version and compiler version do not match exactly
(even down to commit drift between releases). This lets the
runtime conditionally segregate its copies of the libDemangling
symbols from those in the compiler using an inline namespace
without affecting usage throughout source.
Move differentiation-related SILOptimizer files to
{include/swift,lib}/SILOptimizer/Differentiation/.
This reduces directory nesting and gathers files together.
* Update Devirtualizer's analysis invalidation
castValueToABICompatibleType can change CFG, Devirtualizer uses this api but doesn't check if it modified the cfg
MSVC does not realize that the switch is exhaustive and requires that
the path is explicitly marked as unreachable. This silences the C4715
warning ("not all control paths return a value").
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.
Make `SynthesizedFileUnit` attached to a `SourceFile`. This seemed like the
least ad-hoc approach to avoid doing unnecessary work for other `FileUnit`s.
TBDGen: when visiting a `SourceFile`, also visit its `SynthesizedFileUnit` if
it exists.
Serialization: do not treat `SynthesizedFileUnit` declarations as xrefs when
serializing the companion `SourceFile`.
Resolves TF-1239: AutoDiff test failures.
JVP functions are forward-mode derivative functions. They take original
arguments and return original results and a differential function. Differential
functions take derivatives wrt arguments and return derivatives wrt results.
`JVPEmitter` is a cloner that emits JVP and differential functions at the same
time. In JVP functions, function applications are replaced with JVP function
applications. In differential functions, function applications are replaced
with differential function applications.
In JVP functions, each basic block takes a differential struct containing callee
differentials. These structs are consumed by differential functions.
Make `ADContext` lazily create a `SynthesizedFileUnit` instead of creating one
during `ADContext` construction. This avoids always creating a
`SynthesizedFileUnit` in every module, since differentiation is a mandatory
transform that always runs.
It was nonetheless useful to test always creating a `SynthesizedFileUnit` for
testing purposes.
Add implicit declarations generated by the differentiation transform to a
`SynthesizedFileUnit` instead of an ad-hoc pre-existing `SourceFile`.
Resolves TF-1232: type reconstruction for AutoDiff-generated declarations.
Previously, type reconstruction failed because retroactively adding declarations
to a `SourceFile` did not update name lookup caches.
`PullbackEmitter` is a visitor that emits pullback functions. It implements
reverse-mode automatic differentiation, along with `VJPEmitter`.
Pullback functions take derivatives with respect to outputs and return
derivatives with respect to inputs. Every active value/address in an original
function has a corresponding adjoint value/buffer in the pullback function.
Pullback functions consume pullback structs and predecessor enums constructed
by VJP functions.
`VJPEmitter` is a cloner that emits VJP functions. It implements reverse-mode
automatic differentiation, along with `PullbackEmitter`.
`VJPEmitter` clones an original function, replacing function applications with
VJP function applications. In VJP functions, each basic block takes a pullback
struct (containing callee pullbacks) and produces a predecessor enum: these data
structures are consumed by pullback functions.
`LinearMapInfo` contains information about linear map structs and branching
trace enums, which are auxiliary data structures created by the differentiation
transform.
These data structures are constructed in JVP/VJP functions and consumed in
differential/pullback functions.
Canonicalizes `differentiable_function` instructions by filling in missing
derivative function operands.
Derivative function emission rules, based on the original function value:
- `function_ref`: look up differentiability witness with the exact or a minimal
superset derivative configuration. Emit a `differentiability_witness_function`
for the derivative function.
- `witness_method`: emit a `witness_method` with the minimal superset derivative
configuration for the derivative function.
- `class_method`: emit a `class_method` with the minimal superset derivative
configuration for the derivative function.
If an *actual* emitted derivative function has a superset derivative
configuration versus the *desired* derivative configuration, create a "subset
parameters thunk" to thunk the actual derivative to the desired type.
For `differentiable_function` instructions formed from curry thunk applications:
clone the curry thunk (with type `(Self) -> (T, ...) -> U`) and create a new
version with type `(Self) -> @differentiable (T, ...) -> U`.
Progress towards TF-1211.
The differentiation transform does the following:
- Canonicalizes differentiability witnesses by filling in missing derivative
function entries.
- Canonicalizes `differentiable_function` instructions by filling in missing
derivative function operands.
- If necessary, performs automatic differentiation: generating derivative
functions for original functions.
- When encountering non-differentiability code, produces a diagnostic and
errors out.
Partially resolves TF-1211: add the main canonicalization loop.
To incrementally stage changes, derivative functions are currently created
with empty bodies that fatal error with a nice message.
Derivative emitters will be upstreamed separately.
* [Typechecker] Allow enum cases without payload to witness a static get-only property with Self type protocol requirement
* [SIL] Add support for payload cases as well
* [SILGen] Clean up comment
* [Typechecker] Re-enable some previously disabled witness matching code
Also properly handle the matching in some cases
* [Test] Update typechecker tests with payload enum test cases
* [Test] Update SILGen test
* [SIL] Add two FIXME's to address soon
* [SIL] Emit the enum case constructor unconditionally when an enum case is used as a witness
Also, tweak SILDeclRef::getLinkage to update the 'limit' to 'OnDemand' if we have an enum declaration
* [SILGen] Properly handle a enum witness in addMethodImplementation
Also remove a FIXME and code added to workaround the original bug
* [TBDGen] Handle enum case witness
* [Typechecker] Fix conflicts
* [Test] Fix tests
* [AST] Fix indentation in diagnostics def file
Devirtualizing try_apply modified the CFG, but passes that run
devirtualization were not invalidating any CFG analyses, such as the
domtree.
This could hypothetically have caused miscompilation, but will be
caught by running with -sil-verify-all.
If EscapeAnalysis verification runs on unreachable code, it asserts
with "Missing escape connection graph mapping" because the connection
graph builder only runs on reachable blocks.
Add a ReachableBlocks utility and use it during verification.
Fixes <rdar://problem/60373501> EscapeAnalysis crashes with CFG with
unreachable blocks
We need this anyways for -Onone and I want to do some experiments with running
this very early so I can expose more of the stdlib (modulo inlining) to the new
ownership optimizing passes.
I also changed how the inliner handles inlining around OSSA by changing it to
check early that if the caller is in ossa, then we only inline if all of the
callees that the caller calls are in ossa. The intention is to hopefully avoid
weird swings in code-size/perf due to the inliner heuristic's calculation being
artificially manipulated due to some callees not being available to inline (due
to this difference) when others are already available.
* Simplified the logic for creating static initializers and constant folding for global variables: instead of creating a getter function, directly inline the constant value into the use-sites.
* Wired up the constant folder in GlobalOpt, so that a chains for global variables can be propagated, e.g.
let a = 1
let b = a + 10
let c = b + 5
* Fixed a problem where we didn't create a static initializer if a global is not used in the same module. E.g. a public let variable.
* Simplified the code in general.
rdar://problem/31515927
I was inconsistently providing initialized or uninitialized memory
to the callback when projecting a settable address, depending on
component type. We should always provide an uninitialized address.
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.
I was inconsistently providing initialized or uninitialized memory
to the callback when projecting a settable address, depending on
component type. We should always provide an uninitialized address.
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.
