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
Backed off of the implementation with a shared body block, which got
messy really fast with multiple patterns containing address-only values
to pass in, as it led to separated stack alloc/cleanup requirements and
passing stack alloc’d values as block args, aliasing of addresses for
stack cleanup verifying, etc. A mess.
This now does the conservative thing of repeating the body in each
matching pattern block.
Parser now accepts multiple patterns in switch cases that contain variables.
Every pattern must contain the same variable names, but can be in arbitrary
positions. New error for variable that doesn't exist in all patterns.
Sema now checks cases with multiple patterns that each occurence of a variable
name is bound to the same type. New error for unexpected types.
SILGen now shares basic blocks for switch cases that contain multiple
patterns. That BB takes incoming arguments from each applicable pattern match
emission with the specific var decls for the pattern that matched.
Added tests for all three of these, and some simple IDE completion
sanity tests.
Similarly to how we've always handled parameter types, we
now recursively expand tuples in result types and separately
determine a result convention for each result.
The most important code-generation change here is that
indirect results are now returned separately from each
other and from any direct results. It is generally far
better, when receiving an indirect result, to receive it
as an independent result; the caller is much more likely
to be able to directly receive the result in the address
they want to initialize, rather than having to receive it
in temporary memory and then copy parts of it into the
target.
The most important conceptual change here that clients and
producers of SIL must be aware of is the new distinction
between a SILFunctionType's *parameters* and its *argument
list*. The former is just the formal parameters, derived
purely from the parameter types of the original function;
indirect results are no longer in this list. The latter
includes the indirect result arguments; as always, all
the indirect results strictly precede the parameters.
Apply instructions and entry block arguments follow the
argument list, not the parameter list.
A relatively minor change is that there can now be multiple
direct results, each with its own result convention.
This is a minor change because I've chosen to leave
return instructions as taking a single operand and
apply instructions as producing a single result; when
the type describes multiple results, they are implicitly
bound up in a tuple. It might make sense to split these
up and allow e.g. return instructions to take a list
of operands; however, it's not clear what to do on the
caller side, and this would be a major change that can
be separated out from this already over-large patch.
Unsurprisingly, the most invasive changes here are in
SILGen; this requires substantial reworking of both call
emission and reabstraction. It also proved important
to switch several SILGen operations over to work with
RValue instead of ManagedValue, since otherwise they
would be forced to spuriously "implode" buffers.
