This patch moves the ownership of profiling state from SILGenProfiling
to SILFunction, where it always belonged. Similarly, it moves ownership
of the profile reader from SILGenModule to SILModule.
The refactor sets us up to fix a few outstanding code coverage bugs and
does away with sad hacks like ProfilerRAII. It also allows us to locally
guarantee that a profile counter increment actually corresponds to the
SILFunction at hand.
That local guarantee causes a bugfix to accidentally fall out of this
refactor: we now set up the profiling state for delayed functions
correctly. Previously, we would set up a ProfilerRAII for the delayed
function, but its counter increment would never be emitted :(. This fix
constitutes the only functional change in this patch -- the rest is NFC.
As a follow-up, I plan on removing some dead code in the profiling
logic and fixing a few naming inconsistencies. I've left that for later
to keep this patch simple.
This presents a regression in diagnostic quality that is definitely
worth it not to lie to SILGen about whether a switch is covered or not.
At the same time, disable SIL’s unreachable diagnostic for ‘default’
clauses which would previously cause a warning to be emitted if the
default was proven to be unreachable. This analysis is incomplete
anyways and can be done by Sema in the future if we desire.
This will likely become obsolete once @jrose-apple's exhaustive enum work
lands, but I want to use the new resilience expansion in at least a couple
of places to test the functionality.
Cuts off a crash in SILGen for switches over uninhabited types.
We can take this a step further and extend the definition of
"uninhabited" to product types with uninhabited components.
Sometimes in SILGenPattern, we need use an indirect cast on object that does
not require re-abstraction as an optimization. A notable case where this happens
are various casts related to NSError. In such a case, if we have a borrowed cast
operand, perform a copy before the store to preserve semantic sil invariants.
rdar://31880847
This is a loop that attempts to find the index of something with the longest
prefix, but wasn't actually saving the new longest length when finding something
with a longer length, meaning the loop always chose the last index. The update was
accidentally lost in f40e9ef0ae.
We silently miscompiled previously the following code:
protocol Gesture {}
struct Foo {}
struct Bar {}
enum FooOrBar {
case foo(Foo)
case bar(Bar)
}
func main(_ f : FooOrBar) {
switch f {
case .foo(let data as Gesture),
.bar(let data as Gesture):
...
}
...
}
This was because the multiple pattern per case code never implemented support
for address only types.
Now instead of miscompiling such programs, we do the following:
1. We emit an error.
2. When we construct the arguments for the named bindings, we just skip the
address only types. Everything else is normal.
3. In the case block, we use a SILUndef for the address only value.
This ensures that we preserve as many other diagnostics as possible.
rdar://32525952
P.S. As an additional benefit, this eliminates a source of address phi nodes
from SILGen.
Use 'hasAssociatedValues' instead of computing and discarding the
interface type of an enum element decl. This change has specifically not
been made in conditions that use the presence or absence of the
interface type, only conditions that depend on the presence or absence
of associated values in the enum element decl.
Some things noticed by inspection:
- Old spelling of Optional.some
- pattern dump was using the wrong iterator variable and would crash
- addColumns is never used because the row matrix doesn’t expand tuples
We swapped the pattern variables at the wrong level, leaving them bound incorrectly on the cleanup path through a failed 'where' clause check. Fixes rdar://problem/31539726.
This means inserting an eager copy on the switch operand, eliminating the
unforwarding, and destroying the copy in the default case as well as the normal
cases.
I did not refactor this code to use the switch enum builder, but at some point
it should be refactored as such.
The reason I am doing this is that I am going to in the next couple of commits
change enum element dispatch to with or without semantic SIL use proper
ownership.
In this commit, I just did the copy and eliminated any parts of the code that
were predicated on having an address.
This is NFC since all changes are behind a flag. I took a look at the codegen
when this is enabled. It looks pretty optimizable for a -Onone ARC pass. Until I
have that pass though I need this behind a flag.
Basically you see a lot of this pattern:
%0 = begin_borrow %foo
%1 = copy_value %0
...
bb1:
%2 = begin_borrow %1
%3 = copy_value %2
...
destroy_value %3
end_borrow %2 from %1
destroy_value %1
end_borrow %0 from %foo
...
bb2:
destroy_value %1
end_borrow %0 from %foo
...
This is really easy to optimize since one can easily see that all of %1's users
are borrows or a final destroy.
rdar://31145255
Previously, we would put a destroy_value directly on the value that we tried to
cast. Since checked_cast_br is consuming, this would cause the destroy_value on
the failure path to be flagged as a double consume.
This commit causes SILGen to emit the value consumed by checked_cast_br as an
@owned argument to the failure BB, allowing semantic arc rules to be respected.
As an additional benefit, I also upgraded the ownership_model_eliminator test to
use semantic sil verification.
One issue that did come up though is that I was unable to use the new code in
all locations in the compiler. Specifically, there is one location in
SILGenPattern that uses argument unforwarding. I am going to need to undo
argument unforwarding in SILGenPattern in order to completely eliminate the old
code path.
The `cmv.getValue() == value` check here seems fishy, since the downstream case block should always take ownership of the bound values. Now that `copy_value`s produce new defs, it's also never true when it would've been true using the `retain/release_value` model, leading us to fail to copy values when necessary.
Storing this separately is unnecessary since we already
serialize the enum element's interface type. Also, this
eliminates one of the few remaining cases where we serialize
archetypes during AST serialization.
Most of this involved sprinkling ValueOwnershipKind::Owned in many places. In
some of these places, I am sure I was too cavalier and I expect some of them to
be trivial. The verifier will help me to track those down.
On the other hand, I do expect there to be some places where we are willing to
accept guaranteed+trivial or owned+trivial. In those cases, I am going to
provide an aggregate ValueOwnershipKind that will then tell SILArgument that it
should disambiguate using the type. This will eliminate the ackwardness from
such code.
I am going to use a verifier to fix such cases.
This commit also begins the serialization of ValueOwnershipKind of arguments,
but does not implement parsing of value ownership kinds. That and undef are the
last places that we still use ValueOwnershipKind::Any.
rdar://29791263
We preserve the current behavior of assuming Any ownership always and use
default arguments to hide this change most of the time. There are asserts now in
the SILBasicBlock::{create,replace,insert}{PHI,Function}Argument to ensure that
the people can only create SILFunctionArguments in entry blocks and
SILPHIArguments in non-entry blocks. This will ensure that the code in tree
maintains the API distinction even if we are not using the full distinction in
between the two.
Once the verifier is finished being upstreamed, I am going to audit the
createPHIArgument cases for the proper ownership. This is b/c I will be able to
use the verifier to properly debug the code. At that point, I will also start
serializing/printing/parsing the ownershipkind of SILPHIArguments, but lets take
things one step at a time and move incrementally.
In the process, I also discovered a CSE bug. I am not sure how it ever worked.
Basically we replace an argument with a new argument type but return the uses of
the old argument to refer to the old argument instead of a new argument.
rdar://29671437
Changes:
* Terminate all namespaces with the correct closing comment.
* Make sure argument names in comments match the corresponding parameter name.
* Remove redundant get() calls on smart pointers.
* Prefer using "override" or "final" instead of "virtual". Remove "virtual" where appropriate.
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.
Before this commit all code relating to handling arguments in SILBasicBlock had
somewhere in the name BB. This is redundant given that the class's name is
already SILBasicBlock. This commit drops those names.
Some examples:
getBBArg() => getArgument()
BBArgList => ArgumentList
bbarg_begin() => args_begin()
This reverts commit 9508fdc8a7 from #5094. Reemitting
case bodies doesn't work if the body contains nested closures or
declarations, since SILGen expects these to only ever be visited once.
Keep in mind that these are approximations that will not impact correctness
since in all cases I ensured that the SIL will be the same after the
OwnershipModelEliminator has run. The cases that I was unsure of I commented
with SEMANTIC ARC TODO. Once we have the verifier any confusion that may have
occurred here will be dealt with.
rdar://28685236
Today, loads and stores are treated as having @unowned(unsafe) ownership
semantics. This leaves the user to specify ownership changes on the loaded or
stored value independently of the load/store by inserting ARC operations. With
the change to Semantic SIL, this will no longer be true. Instead loads, stores
have ownership semantics that one must reason about such as copy, take, and
trivial.
This change moves us closer to that world by eliminating the default
OwnershipQualification argument from create{Load,Store}. This means that the
compiler developer cannot ignore reasoning about the ownership semantics of the
memory operation that they are creating.
Operationally, this is a NFC change since I have just gone through the compiler
and updated all places where we create loads, stores to pass in the former
default argument ({Load,Store}OwnershipQualifier::Unqualified), to
SILBuilder::create{Load,Store}(...). For now, one can just do that in situations
where one needs to create loads/stores, but over time, I am going to tighten the
semantics up via the verifier.
rdar://28685236
