This could probably be an error, but I am leaving it to preserve previous
behavior. The reason I am hoisting this is that I am hoisting it before we
potentially split a basic block. When we evaluate if we can early exit, we
should never do work before we know that we will emit /something/.
This code today only handles the following two instructions:
1. checked_cast_addr_br
2. unconditional_checked_cast_addr
We were asserting that the source/dest of these values were addresses... but
they are obviously are!
I think that this is a result of previous refactorings. This code has a long
time assert in it that Src (since 2016) is an address and for a long time the bridged type has
always been an object (since 2015). So, thus we know that the following is always true.
SILBridgedTy != Src->getType()
I also removed a level of indentation. NFC.
The 2015 commit hash is: 2dd38eee0e (the original commit).
The 2016 commit hash is: 506ab9809f
For context, String, Nil, Bool, and Int already behave this way.
Note: Swift can compile against 80 or 64 bit floats as the builtin
literal type. Thus, it was necessary to capture this bit somehow in the
FloatLiteralExpr. This was done as another Type field capturing this
info.
Where possible, pass around a ClassDecl or a CanType instead of a
SILType that might wrap a metatype; the unwrapping logic was
repeated in several places.
Also add a FIXME for a bug I found by inspection.
I am going to use this to refactor a bunch of the goop in the cast optimizer. At
a high level, we are really just performing a giant switch over the casts to
grab different state. We then take that state and we pass it into the bridge
cast optimizer.
To make such code more compact/easier to understand, I am adding in this commit
a type erased dynamic cast instruction type called "SILDynamicCastInst". In
subsequent commits, I wire up each of the individual instructions to it one at a
time.
As an additional advantage it will enable us to take advantage of covered
switches when ever in the future we introduce new casts.
This utility is generally a horrible idea but even worse the
callers were not doing anything to ensure the required
invariants actually held.
Add a new canReplaceLoadSequence() method and chek it in the
right places.
For context, String, Nil, and Bool already behave this way.
Note: Before it used to construct (call, ... (integer_literal)), and the
call would be made explicit / implicit based on if you did eg: Int(3) or
just 3. This however did not translate to the new world so this PR adds
a IsExplicitConversion bit to NumberLiteralExpr. Some side results of
all this are that some warnings changed a little and some instructions are
emitted in a different order.
I also ported the constant_propagation.sil tests over for ownership and updated
a few parts of the cast optimizer so that those tests pass with and without
ownership. I purposely only updated the parts of the cast optimizer that crashed
with ownership in the relevant test so that I can add new sil code coverage for
those uncovered code paths.
This method wasn’t returning the protocol on which the that the witness
method would satisfy, as documented. Rather, it was returning the protocol
to which the `Self` type conforms, which could be completely unrelated. For
example, in IndexingIterator’s conformance to IteratorProtocol, this method
would produce the protocol “Collection”, because that’s where the witness
itself was implemented. However, there isn’t necessarily a single such
protocol, so checking for/returning a single protocol was incorrect.
It turns out that there were only a few SIL verifier assertions of it
(that are trivially true) and two actual uses in code:
(1) The devirtualizer was using this computation to decide when it didn’t
need to perform any additional substitutions, but it’s predicate for doing
so was essentially incorrect. Instead, it really wanted to check whether
the Self type is still a type parameter.
(2) Our polymorphic convention was using it to essentially check whether
the ’Self’ instance type of a witness_method was a GenericTypeParamType,
which we can check directly.
Fixes rdar://problem/47767506 and possibly the hard-to-reproduce
rdar://problem/47772899.
replace value uses action to make sure that we properly notify passes that we
made the change.
This fixes a latent bug where we were not notifying SILCombine about this
replacement.
Specifically:
- auto replaceCast = [&](SingleValueInstruction *NewCast) {
- assert(Ty.getAs<AnyMetatypeType>()->getRepresentation()
- == NewCast->getType().getAs<AnyMetatypeType>()->getRepresentation());
- MCI->replaceAllUsesWith(NewCast);
- EraseInstAction(MCI);
- return NewCast;
+ auto replaceCast = [&](SILValue newValue) -> SILValue {
+ assert(ty.getAs<AnyMetatypeType>()->getRepresentation() ==
+ newValue->getType().getAs<AnyMetatypeType>()->getRepresentation());
+ ReplaceValueUsesAction(mci, newValue);
+ EraseInstAction(mci);
+ return newValue;
};
Notice how we use MCI->replaceAllUsesWith instead of one of our replace call
backs. SILCombine hooks these to know if it should re-run users.
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.
Beside fixing the compiler crash, this change also improves the stack-nesting correction mechanisms in the inliners:
* Instead of trying to correct the nesting after each inlining of a callee, correct the nesting once when inlining is finished for a caller function.
This fixes a potential compile time problem, because StackNesting iterates over the whole function.
In worst case this can lead to quadratic behavior in case many begin_apply instructions with overlapping stack locations are inlined.
* Because we are doing it only once for a caller, we can remove the complex logic for checking if it is necessary.
We can just do it unconditionally in case any coroutine gets inlined.
The inliners iterate over all instruction of a function anyway, so this does not increase the computational complexity (StackNesting is roughly linear with the number of instructions).
rdar://problem/47615442
I discovered this due to the mandatory inliner doing devirtualization. I ported
all of the relevant SIL tests to increase code coverage of this code when
ownership is enabled.
