Each call site will soon have to think about passing in the right expansion
instead of just assuming the default will be OK. But there are now only a
few call sites left, because most have been refactored to use convenience
APIs that pass in the right resilience expansion already.
In a previous commit, I banned in the verifier any SILValue from producing
ValueOwnershipKind::Any in preparation for this.
This change arises out of discussions in between John, Andy, and I around
ValueOwnershipKind::Trivial. The specific realization was that this ownership
kind was an unnecessary conflation of the a type system idea (triviality) with
an ownership idea (@any, an ownership kind that is compatible with any other
ownership kind at value merge points and can only create). This caused the
ownership model to have to contort to handle the non-payloaded or trivial cases
of non-trivial enums. This is unnecessary if we just eliminate the any case and
in the verifier separately verify that trivial => @any (notice that we do not
verify that @any => trivial).
NOTE: This is technically an NFC intended change since I am just replacing
Trivial with Any. That is why if you look at the tests you will see that I
actually did not need to update anything except removing some @trivial ownership
since @any ownership is represented without writing @any in the parsed sil.
rdar://46294760
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
Mostly functionally neutral:
- may fix latent bugs.
- may reduce useless basic blocks after inlining.
This rewrite encapsulates the cloner's internal state, providing a
clean API for the CRTP subclasses. The subclasses are rewritten to use
the exposed API and extension points. This makes it much easier to
understand, work with, and extend SIL cloners, which are central to
many optimization passes. Basic SIL invariants are now clearly
expressed and enforced. There is no longer a intricate dance between
multiple levels of subclasses operating on underlying low-level data
structures. All of the logic needed to keep the original SIL in a
consistent state is contained within the SILCloner itself. Subclasses
only need to be responsible for their own modifications.
The immediate motiviation is to make CFG updates self-contained so
that SIL remains in a valid state. This will allow the removal of
critical edge splitting hacks and will allow general SIL utilities to
take advantage of the fact that we don't allow critical edges.
This rewrite establishes a simple principal that should be followed
everywhere: aside from the primitive mutation APIs on SIL data types,
each SIL utility is responsibile for leaving SIL in a valid state and
the logic for doing so should exist in one central location.
This includes, for example:
- Generating a valid CFG, splitting edges if needed.
- Returning a valid instruction iterator if any instructions are removed.
- Updating dominance.
- Updating SSA (block arguments).
(Dominance info and SSA properties are fundamental to SIL verification).
LoopInfo is also somewhat fundamental to SIL, and should generally be
updated, but it isn't required.
This also fixes some latent bugs related to iterator invalidation in
recursivelyDeleteTriviallyDeadInstructions and SILInliner. Note that
the SILModule deletion callback should be avoided. It can be useful as
a simple cache invalidation mechanism, but it is otherwise bug prone,
too limited to be very useful, and basically bad design. Utilities
that mutate should return a valid instruction iterator and provide
their own deletion callbacks.
To do so this commit does a few different things:
1. I changed SILOptFunctionBuilder to notify the pass manager's logging
functionality when new functions are added to the module and to notify analyses
as well. NOTE: This on purpose does not put the new function on the pass manager
worklist since we do not want to by mistake introduce a large amount of
re-optimizations. Such a thing should be explicit.
2. I eliminated SILModuleTransform::notifyAddFunction. This just performed the
operations from 1. Now that SILOptFunctionBuilder performs this operation for
us, it is not needed.
3. I changed SILFunctionTransform::notifyAddFunction to just add the function to
the passmanager worklist. It does not need to notify the pass manager's logging
or analyses that a new function was added to the module since
SILOptFunctionBuilder now performs that operation. Given its reduced
functionality, I changed the name to addFunctionToPassManagerWorklist(...). The
name is a little long/verbose, but this is a feature since one should think
before getting the pass manager to rerun transforms on a function. Also, giving
it a longer name calls out the operation in the code visually, giving this
operation more prominance when reading code. NOTE: I did the rename using
Xcode's refactoring functionality!
rdar://42301529
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
This commit does not modify those APIs or their usage. It just:
1. Moves the APIs onto SILFunctionBuilder and makes SILFunctionBuilder a friend
of SILModule.
2. Hides the APIs on SILModule so all users need to use SILFunctionBuilder to
create/destroy functions.
I am doing this in order to allow for adding/removing function notifications to
be enforced via the type system in the SILOptimizer. In the process of finishing
off CallerAnalysis for FSO, I discovered that we were not doing this everywhere
we need to. After considering various other options such as:
1. Verifying after all passes that the notifications were sent correctly and
asserting. Turned out to be expensive.
2. Putting a callback in SILModule. This would add an unnecessary virtual call.
I realized that by using a builder we can:
1. Enforce that users of SILFunctionBuilder can only construct composed function
builders by making the composed function builder's friends of
SILFunctionBuilder (notice I did not use the word subclass, I am talking
about a pure composition).
2. Refactor a huge amount of code in SILOpt/SILGen that involve function
creation onto a SILGenFunctionBuilder/SILOptFunctionBuilder struct. Many of
the SILFunction creation code in question are straight up copies of each
other with small variations. A builder would be a great way to simplify that
code.
3. Reduce the size of SILModule.cpp by 25% from ~30k -> ~23k making the whole
file easier to read.
NOTE: In this commit, I do not hide the constructor of SILFunctionBuilder since
I have not created the derived builder structs yet. Once I have created those in
a subsequent commit, I will hide that constructor.
rdar://42301529
This patch both makes debug variable information it optional on
alloc_stack and alloc_box instructions, and forced variable
information on debug_value and debug_value_addr instructions. The
change of the interface uncovered a plethora of bugs in SILGen,
SILTransform, and IRGen's LoadableByAddress pass.
Most importantly this fixes the previously commented part of the
DebugInfo/local-vars.swift.gyb testcase.
rdar://problem/37720555
We can just !SILFunction::hasQualifiedOwnership(). Plus as Andy pointed out,
even ignoring the functional aspects, having APIs with names this close can
create confusion.
introduce a common superclass, SILNode.
This is in preparation for allowing instructions to have multiple
results. It is also a somewhat more elegant representation for
instructions that have zero results. Instructions that are known
to have exactly one result inherit from a class, SingleValueInstruction,
that subclasses both ValueBase and SILInstruction. Some care must be
taken when working with SILNode pointers and testing for equality;
please see the comment on SILNode for more information.
A number of SIL passes needed to be updated in order to handle this
new distinction between SIL values and SIL instructions.
Note that the SIL parser is now stricter about not trying to assign
a result value from an instruction (like 'return' or 'strong_retain')
that does not produce any.
The issue here is that we know that the given value is already borrowed, so
technically, we do not need a copy. The problem is that there is still
potentially a destroy of the value that will be cloned by the closure cloner.
This results in use-after-frees.
This commit fixes the problem by inserting the relevant copy_value and changing
the borrowed value into a +1 copy.
This is a bug that would have been caught by the Ownership verifier since a
borrowed value can not be destroyed (so the verifier would have tripped).
rdar://32625475
I found this bug by inspection.
This is an important bug to fix since this pass runs at -Onone and the bug
results in the compiler hitting an unreachable.
The way the unreachable is triggered is that when we detect that we are going to
promote a box, if we see a (struct_element_addr (project_box box)), we don't map
the struct_element_addr to a cloned value. If we have a load, this is not an
issue, since we are mapping the load to the struct_extract. But if we have /any/
other non-load users of the struct_element_addr, the cloner will attempt to look
up the struct_element_addr and will be unable to find it, hitting an
unreachable.
rdar://32776202
Till now createApply, createTryApply, createPartialApply were taking some arguments like SubstCalleeType or ResultType. But these arguments are redundant and can be easily derived from other arguments of these functions. There is no need to put the burden of their computation on the clients of these APIs.
The removal of these redundant parameters simplifies the APIs and reduces the possibility of providing mismatched types by clients, which often happened in the past.
The assert should have been checking for both guaranteed and trivial, not just
guaranteed.
This also uncovered a hole in the testing for capture promotion. I added the
missing test.
<rdar://problem/32027013>
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.
At some point, pass definitions were heavily macro-ized. Pass
descriptive names were added in two places. This is not only redundant
but a source of confusion. You could waste a lot of time grepping for
the wrong string. I removed all the getName() overrides which, at
around 90 passes, was a fairly significant amount of code bloat.
Any pass that we want to be able to invoke by name from a tool
(sil-opt) or pipeline plan *should* have unique type name, enum value,
commend-line string, and name string. I removed a comment about the
various inliner passes that contradicted that.
Side note: We should be consistent with the policy that a pass is
identified by its type. We have a couple passes, LICM and CSE, which
currently violate that convention.
