ManagedValue::ensurePlusOne is already enabled.
I also tightened up the implementation a little bit by:
1. Fixed a typo in ManagedValue::isPlueZero(). I forgot to negate a boolean
=/. Luckily, I have been using isPlusOne() instead of isPlusZero for all queries
I have needed so far.
2. I added code to ManagedValue/RValue so that if we have SILUndef, we do not
emit copies on SILUndef when we perform ensurePlusOne().
3. I changed isPlusOne() and isPlusZero() to return true for SILUndef. SILUndef
has "any" ownership so it is compatible with all forms of ownership.
rdar://34222540
This was found by the ownership verifier in the tuple shuffle test. The specific
problem was that we were performing an ensurePlusOne and forwarding that
value. The rvalue code was expecting this to be a "true" forward. Rather than
relying on the rvalue code's assumptions, this commit refactors the tuple
implosion code to just return an explicit ManagedValue with the proper cleanup
upon it.
Bug caught by the ownership verifier running on the argument shuffle SILGen test
with +0 enabled.
*NOTE* This is not a problem on any of the release branches, since ensurePlusOne
is a no-op on those branches (I just turned it on a couple of days ago).
rdar://34222540
We already have this requirement on RValues, so it is natural to extend it to
ManagedValue. The main reason why I am doing this though is to tighten up SILGen
asserts to help me catch places where we forward +0 arguments into memory
without a copy/cleanup.
rdar://34222540
In the code, we assume generally that a value that is "forwarded" into memory is
forwarded at +1. This isn't enforced and when I tried to enforce it at +0, I
quickly hit assertion failures. So rather than fix the myriad places, I am using
ensurePlusOne to handle such cases without needing to change a bunch of the
code.
rdar://34222540
This helper function returns *this if the value is already at +1. Otherwise, if
the value is a +0 value, we copy the value. Since I am using this initially for
some experiments, I am hiding it behind the enable guaranteed normal arguments
flag.
rdar://34222540
I am currently changing Callee to use an ArgumentSource instead of a SILValue to
represent self. Due to uncurrying/etc in certain cases, we need to communicate
that a value needs to be borrowed later before use. To do that in a clean way, I
am introducing a new form of ArgumentSource::Kind, DelayedBorrowedRValue.
This type of ArgumentSource can only be produced by calling
ArgumentSource::delayedBorrow(...). Such an argument source acts like a normal
RValue, except when you perform asKnownRValue, a borrow is performed before
returning the known rvalue.
Once uncurrying is ripped out/redone, this code can be removed in favor of just
using a borrowed ArgumentSource.
rdar://33358110
Today, SILGenFunction::emitRValue assumes the caller will create any cleanup
scopes that are needed to cleanup side-effects relating to the rvalue
evaluation. The API also provides the ability for the caller to specify that a
+0 rvalues is an "ok" result. The API then tries to produce a +0 rvalue and
returns a +1 rvalue otherwise. These two properties create conflicting
requirements on the caller since the caller does not know whether or not it
should create a scope (if a +1 rvalue will be returned) or not (if a +0 rvalue
would be returned).
The key issue here is the optionality of returning a +0 rvalue. This change
begins to resolve this difference by creating two separate APIs that guarantee
to the caller whether or not a +0 or a +1 rvalue is returned and also creates
local scopes for the caller as appropriate. So by using these APIs, the caller
knows that the +0 or +1 rvalue that is returned has properly been put into the
caller scope. So the caller no longer needs to create its own scopes anymore.
emitPlusOneRValue is emitRValue except that it scopes the rvalue emission and
then *pushes* the produced rvalue through the scope. emitPlusZeroRValue is
currently a stub implementation that just calls emitPlusOneRValue and then
borrows the resulting +1 RValue in the outer scope, creating the +0 RValue that
was requested by the caller.
rdar://33358110
This is already an RValue invariant that used to be enforced upon RValue
construction. We put in a hack to work around a bug where that was not occuring
and changed RValue constructors to instead load stored objects when they needed
to. But the problem is that since then we have added more constructors that
provide other manners to create such an invalid RValue.
I added verification to many parts of RValue and exposed an additional verify
method that we can invoke at the end of emitRValue() eventually to verify our
invariants. This will give me the comfort to make that assumption in other parts
of SILGen without worry.
I also performed a small amount of cleanup of RValue construction.
rdar://33358110
This can only come up if you have a tuple that is part address only and part
non-trivial, but loadable. Previously, the non-trivial value would be taken but
no cleanup would be created. This would trip the ownership verifier. Now we
properly model this via a load_borrow.
*NOTE* Due to us modeling a take as not-copying a value, after ownership is
stripped we have the same underlying singular load, so this is not a bug, but a
semantic violation of the model (i.e. taking a +0 self value).
rdar://33358110
I already in a previous commit forced all rvalues to have consistent cleanups
and consistent value ownership kinds. Now that we know all constructed RValues
are consistent, we can safely query that information.
rdar://33358110
This means that all non-trivial ManagedValues must:
1. All have a cleanup or all not have a cleanup.
2. Have the same ValueOwnershipKind.
Semantic SIL requires this of tuple values. So it makes sense to catch this bug
as early as possible.
rdar://33358110
We're now double-diagnosing some things that are caught by both
SILGen and static enforcement; we can fix that later, but I want to
unblock this problem first.
The reason that this is being done is that:
1. SILGenFunction is passed around all throughout SILGen, including in between
APIs some of which call the SILGenFunction variable SGF and others that call it
gen.
2. Thus when one is debugging code in SILGen, one wastes time figuring out what
the variable name of SILGenFunction is in the current frame.
I did not do this by hand. I did this by:
1. Grepping for "SILGenFunction &gen".
2. By hand inspecting that the match was truly a SILGenFunction &gen site.
3. If so, use libclang tooling to rename the variable to SGF.
So I did not update any use sites.
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
The RValue(ArrayRef<ManagedValue>, CanType) constructor was intended as a semi-private interface for building an RValue from a pre-exploded array of elements, but was (understandably) widely being misused as a general ManagedValue-to-RValue constructor, causing crashes when working with tuples in various contexts where RValue's methods expected them to be exploded. Make the constructor private and update most improper uses of it to use the exploding RValue constructor, or to use a new `RValue::withPreExplodedElements` static method that more explicitly communicates the intent of the constructor. Fixes rdar://problem/29500731.
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
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.
If a global variable in a module we are compiling has a type containing
a resilient value type from a different module, we don't know the size
at compile time, so we cannot allocate storage for the global statically.
Instead, we will use a buffer, just like alloc_stack does for archetypes
and resilient value types.
This adds a new SIL instruction but does not yet make use of it.
Michael Gottesman
Davide Italiano
John McCall
practicalswift
Joe Shajrawi
Hugh Bellamy
Joe Groff
Michael Gottesman
Doug Gregor
Erik Eckstein
Slava Pestov
Zach Panzarino