-
Notifications
You must be signed in to change notification settings - Fork 424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposed Language Rules for Expiring Values #13704
Labels
Comments
This was referenced Aug 14, 2019
mppf
added a commit
that referenced
this issue
Jan 2, 2020
Expiring value analysis Following the design discussed in #13704 (and in particular Option 2 there), this PR adds an expiring value analysis to the compiler. This analysis is not currently used except for in the test directory `types/records/expiring/`. It adds flags to variables that will be deinitialized. These flags will be used in a future PR to adjust the point at which deinitialization occurs based on the expiring value analysis. Implementation changes in more detail: * adjusts the way that flattenFunctions handles outer variables in lifetime constraints so that they are still recognizable to the lifetime checker. In particular, avoid adding temps and mark them with a flag. * save some time/trouble by not trying to do lifetime checking in the strings literal module * adjust canonicalization for lifetime checking to prefer variables with `FLAG_INSERT_AUTO_DESTROY` before using the name length as a tie breaker * adjust lifetime checker to, when analyzing a function, consider arguments marked with `lifetime > outerVariable` to have infinite lifetime * most of the code change is adding the new analysis which takes 2 steps * GatherAliasesVisitor computes the set of potential aliases for local variables * MarkCapturesVisitor decides which of the local variables are potentially captured Reviewed by @vasslitvinov - thanks! - [x] full local futures testing
1 task
This was referenced Jan 14, 2020
mppf
added a commit
that referenced
this issue
Jan 16, 2020
Adjust point of deinitialization based on expiring value analysis This PR updates the compiler to use expiring value analysis in order to choose the point at which `deinit` for a local record variable is run. It does this according to the discussion in #13704. This PR includes several minor modification to the rules described in #13704. 1. Variables referred to in `begin` blocks are considered to expire at the end of the function (and issue #14750 discusses this and how it should interact with`sync` blocks) 2. An exception for `=` and `init=` for managed types. Because ownership transfer from non-nil is one of the use cases, we need the analysis to not consider `x` to potentially alias `y` in the below code, when both are `owned` - `var x = y;`. In general, if these are classes or records, they could share a reference to a field. For `owned` however, the right-hand-side of the initialization is left `nil` after the statement, and so there is no potential for aliasing between `x` and `y` after the statement. Additionally, for `shared`, while `x` and `y` refer to the same memory, this has no impact on when `x` or `y` can be deinitialized or a copy could be avoided. Therefore, when considering if a potential alias is created, the analysis ignores assignment and copy-initialization of managed types. Additionally, it improves the split-init implementation. Previous to this PR, an initialization point like `x = functionReturningByValue()` would cause the compiler to emit `init=` when it did not need to. Additionally, the compiler was emitting `deinit` calls for variables that were declared but not yet initialized if there was a return from the block. Lastly, this PR extends split-init to work with arrays (where before it was disabled in resolution). Lastly, it adjusts the lifetime checker to now require a lifetime clause on `=` or `<=>` overloads for types that contain borrows. Implementation steps: * Remove FnSymbol::replaceReturnSymbol - it is unused * rename `_statementLevelSymbol` to `chpl_statementLevelSymbol` and add `astr_chpl_statementLevelSymbol` to the compiler * Print out variables stored directly in ForallStmts as "other variables" in the AST logs * Add flags to disable split init and early deinit; change fLifetimeChecking to fNoLifetimeChecking to reflect the current default * Move addAutoDestroyCalls to after ret-arg transform so it can run after lifetime checking (where the expiring value analysis runs) and make several adjustments to handle ret-args in addAutoDestroyCalls. * Add code to normalize to mark the end-of-statement. Normalize adds a `PRIM_END_OF_STATEMENT` call after the end of each statement that isn't already at the end of a block or that refers to user variables. `PRIM_END_OF_STATEMENT` arguments are SymExprs referring to user variables that should be preserved until the end of that statement, which matters for code that is removed before callDestructors, such as `x.type`. Adjusted the unordered forall optimization and flatten functions to ignore these `PRIM_END_OF_STATEMENT` calls. These `PRIM_END_OF_STATEMENT` calls are removed in callDestructors. * Updated the normalizer code for split-init to properly handle `x = makeRecord()` in addition to `x = new MyRecord()` and to store the type-expr in a temporary that is passed to both `PRIM_INIT_VAR_SPLIT_DECL` and `PRIM_INIT_VAR_SPLIT_INIT` to enable split init of arrays * Updated the normalizer code for split init to explicitly handle TryStmts even though they currently disable split-init * Updated functionResolution to handle split-init of arrays * addAutoDestroyCalls creates a map from a statement to which variables should be destroyed there in order to implement the early deinit. Any variables not in the map will be destroyed according to previous rules (i.e. at the end of the block). Once deinit calls for these are added, the variable is added to the ignore set. * updated addAutoDestroyCalls and AutoDestroyScope to track which variables have been initialized (in case they are split-inited) and to propagate that information to parent blocks (in case a variable is initialized with split-init in a nested block). Test updates: * updating tests that check deinit occurs for end-of-block variables on return/throw/etc to force some variables to be end-of-block * work-arounds for #14746 * updating tests for new deinit behavior and in some cases test both last-mention and end-of-block * new tests of new behavior Reviewed by @vasslitvinov - thanks! - [x] full local testing Future Work: * document deinit points in the language specification - Cray/chapel-private#690 * deinitialize in reverse initialization order rather than reverse declaration order - Cray/chapel-private#691 Related Design Questions: * Expiring value analysis interaction with `=` and `init=` for managed types (described above) * Lifetime checking and = overloads #14757 * Split-init interactions with deinitialization order #14747 * Should split-init be allowed within a try block? #14748 * Should last mention deinitialization point go inside blocks? #14749 * Expiring value analysis and begin blocks #14750 * Split-init interaction with warning about local-from-distributed #14746 * Should unused record variables be removed? #14758
This was referenced Feb 6, 2020
mppf
added a commit
that referenced
this issue
Feb 19, 2020
Add 1st draft of copy elision This PR adds a 1st draft of copy elision. It works during callDestructors and looks for a copy being the last mention of a variable before the deinitializer is run for that variable. In that event, it elides the copy and does not deinitialize the variable. Copy elision is discussed in issues #13704, #9490, #9421, #14860, #14870, and #14874. This PR is closest to the proposal in #14860 of any of those, but it does not quite match that proposal. In the future, we expect to separate the effort of deciding which copies are elide-able from the work of doing the actual elision. Since the elision involves not deinitializing something, it makes sense to continue to do that in callDestructors. However, the work of deciding which copies are elide-able will move to some part of the compiler before nil checking (because nil checking can find certain errors after an elided copy makes a variable invalid). Additionally we are planning to use a simpler rule for when a copy can be elided (#14874). Reviewed by @benharsh - thanks! - [x] full local testing - [x] full local gasnet testing
1 task
mppf
added a commit
that referenced
this issue
Feb 24, 2020
Errors for ownership transfer from non-nilable owned Resolves #13600. This PR adds compilation errors for ownership transfer from non-nilable owned. * Adjusts setInstantiationPoint to avoid resetting userInstantiationPointLoc when clearing the instantiation point in cleanups * Adds `FLAG_IGNORE_TRANSFER_ERRORS` to disable the new errors on a per-function basis and mark two array implementation functions with these * In checkForErroneousInitCopies, mark functions only called in erroneous copy functions with `FLAG_ERRONEOUS_COPY` so we can avoid raising transfer errors in them * Adds findNonNilableStoringNil, an analysis called in callDestructors and stored in nilChecking. This analysis raises an error for ownership transfer from non-nilable owned except in certain cases. It currently is connected to the expiring value analysis (following #13704) but we may make it stricter. * Fixes findCopyInitFn and findAssignFn for tuples * mark `init=` with `FLAG_ERRONEOUS_COPY` along with initCopy etc in resolution if it contains an error * in errorIfNonNilableType, don't raise errors for variables declared with `FLAG_NO_INIT` * in findLocationIgnoringInternalInlining, avoid an infinite loop if no calls were found to a function * change several TaskErrors methods to return/yield `owned Error?` instead of other types in order to allow the user to throw one of these without generating an ownership transfer error. * in SharedObject, factor clear logic into new method `doClear` so that `clear` can be more aggressively checked than deinit. * Add errors in `list` for copy-initialization from types that cannot be copied. * Adjust `list.pop` to move from the list into the return value rather than copying. This allows `list.pop` to be used on a list storing non-nilable owned. * Adjust quite a few tests to avoid the new errors. - [x] full local testing Reviewed by @benharsh - thanks! Future work: issues #14942 #14941 #14940
mppf
added a commit
that referenced
this issue
Mar 4, 2020
Use simpler rule for deinit point This PR updates the compiler to use simpler rules for when a variable is dead. The expiring value analysis (proposed in #13704) did not end up being as useful as initially expected for copy elision. As a result we are shifting to a simpler design for when copy elision can occur - #14874. That leaves the expiring value analysis impacting the deinitialization point (#11534 #11492) and the ownership transfer from non-nilable checking (#13600). However the ownership transfer checking is not significantly improved with expiring value analysis (it is approximately as effective when handled by the must-alias analysis in the nil checker). That leaves the deinit points. Since a copy elision can cause a variable to be effectively dead in the current scope after it occurs, there is some relationship between deinit points and copy elision. That is why #13704 tried to address these two together. For example: ``` chapel proc acceptsInIntent( in arg ) { ... } proc test() { var x: MyRecord; ... acceptsInIntent(x); // copy elision occurs here // x is effectively dead in this scope // accesses through refs/borrows hopefully are compilation errors // but could be use-after-frees. } ``` However since the copy elision rule is relatively simple, this PR changes the rule for when a variable is dead to simply include the copy elision rule. Besides that, it is based upon the simpler rule in #13704 / #11534 (comment) . Here is the rule as of this PR: > A variable is dead: > * after copy elision if it occurred (after the last mention is > used to copy-initialize a variable or in intent argument) > * for the results of nested call expressions not involved in initializing > a user variable or reference after the end of the statement > * otherwise, at the end of the scope in which it is declared There are two important things to note about the above rule: 1. "involved in initializing a user variable or reference" includes through split-init 2. the above rule applies to module-scope variables (aka global variables) as well as local variables. For global variables Also note that `--no-early-deinit` is available to make all temps deinited end-of-block. This PR takes the following steps to implement the change to deinitialization points: * add a shared helper isInitOrReturn to CallExpr.cpp; use it in possiblyInitsDestroyedVariable * removes --report-expiring flag and the entire expiring value analysis * adjusts the normalizer to leave temps in module init functions (instead of making them global variables) so that this can be determined later (after split-inits are known) * fixes a few bugs with nil checking * adds MarkTempsVisitor / gatherTempsDeadLastMention / markTempsDeadLastMention which are called from fixPrimInitsAndAddCasts in resolveFunction.cpp. This analyzes temp variables by how they are used to determine if they are used to initialize a user variable. Besides adding `FLAG_DEAD_LAST_MENTION` or `FLAG_DEAD_END_OF_BLOCK`, if the variable is in a module initialization function, it will pull it out into global scope. * insertReturnTemps marks the new temps since this runs after MarkTempsVisitor * adjust several tests that have use-after-frees according to the new rules: * nbody_orig_1.chpl * assignRecord.chpl * owned-class-instantiation-types-user-init.chpl * owned-class-instantiation-types.chpl * (and these also which were added after checking in PR #15072 detected an issue with them) * iter-return-first-borrowed-array-element.chpl * owned-record-instantiation-types-user-init.chpl * classes/initializers/generics/inheritance/inherited.chpl Reviewed by @benharsh - thanks! - [x] full local testing
PR #15041 implements a simpler rule for deinit points that enables this program to behave as originally expected.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I see 3 areas that are possible related
In all of these cases, I think the language could benefit from a concept for an expiring value / dead value. This issue shows some examples for each case, discusses two proposals for language rules, and investigates these proposals using examples.
As of 10/1/2019, there is some support for Option 2 in preference to Option 1 because it solves more of the current problems.
Motivating Examples
Motivating examples for placement of deinit on records
I/O deinit example
In the following example (from #11492), one would like the channel created by
tmp.writer()
to be closed before the channel created bytmp.reader()
is created. Otherwise, the reader cannot access data written by the writer. However the current behavior is to destroy all local variable records at the end of the block in which they are declared. The result is thattmp.reader().read(B)
does not find any written data in the file.Slice deinit example
In contrast, the situation with array slices is the opposite, where the temporary needs to be destroyed at the end of the block and not before:
In this example, there is a reference to a call-expression temporary storing the slice, which in turn refers to a call-expression temporary storing the whole array. As a result, the compiler may not deinitialize either the array or the slice array-view of it at point 1. It must perform this deinitialization after these are no longer used, at point 2.
Owned deinit example
Here, if the
new owned
value were deleted after that statement,instance
would refer to freed memory.#11534 contains more examples.
Motivating examples for eliding copies from expiring values
Desire for this has come up in user code with a pattern like
Here
MyClass
is storing an array and we would like its initializer to accept that array within
intent. In that event, we would like the array already constructed to "move" into the class field, rather than copying. The copy can be expensive if it is a distributed array.Already, the compiler can eliminate copies in certain cases, such as
var x = g()
.Related issues are #9421 and #9490.
Motivating example for ownership transfer from non-nil owned values
Because of the role of type inference, it is easy to create variables that are non-nil owned. Simple patterns using these variables will lead to error if we cannot allow a variable of type
owned MyClass
to storenil
, which is the current plan (per #12614). Such patterns are currently simply not checked, which is totally not type-safe and I expect to change.Language Design Options
Option 1
From
#11534 (comment)
A local value is dead at the end of the block if it was created in a
var
/ref
/const
/const ref
variable declaration and dead at the end of the statement otherwise. Globals continue to be destroyed at the end of the program's execution).Pros:
Cons:
f(g())
to work the same asvar x = g(); f(x);
).std::move
store(x, makeOwnedC().borrow());
example below).Option 2
In option 2, the compiler would consider the appearances of a local variable in the source code.
if
conditional or a loop that contained a mention of the variable in its body, the variable would be dead after thatif
or the loop. Globals continue to be destroyed at the end of the program's execution.A function call
f(..., a, ...)
is potentially capturing an alias to an argumenta
if the lifetime constraint or inferred lifetime constraint would allow:a
into another formal argumenta
into an outer / global variablea
(e.g.ref x = a
).A function call
g(..., b, ...)
is potentially creating an alias to an argumentb
if the lifetime constraint or inferred lifetime constraint would allow:b
To determine if a variable
v
is potentially capture, the analysis would develop a set of potential aliases. Initially, the set consists the variablev
only. The analysis would expand this set to include the results of any calls that create an alias, and proceed until this set does not change. Then it would consider calls mentioning members of this set to determine if any of them are potentially capturing.Pros:
f(g())
works the same asvar x = g(); f(x);
).std::move
Cons:
Further Discussion and Examples Evaluating Each Option
Considering the Motivating Examples
I/O deinit example
Under option 1 and option 2, the channel that results from
tmp.writer()
would be deinited (and flushed) at point 1, rather than at point 2 as it is today.In Option 1, it is deinited at point 1 because the temporary was not in a variable declaration.
In Option 2, it s deinited at point 2 because
channel.write()
does not have the potential to save an alias (returns abool
, so can't save it that way; can't store the alias in other arguments or globals because there is no lifetime clause).Slice deinit example
Option 1 would deinit the temporaries at point 1 because they are declared in a variable declaration statement. Option 2 would deinit the temporaries at point 2 because:
makeArray()
as captured by a slice (the slice borrows from the original array)ref slice = ___
.Owned deinit example
Here Option 1 would destroy the temporary storing
new owned C(1)
at the end of the block (point 2) because it was in a variable declaration statement. Option 2 would also destroy it there because an alias to that temporary is created (and stored ininstance
).Motivating examples for eliding copies from expiring values
Option 1 does not allow for eliding the copy in this case. Option 2 can elide the copy. Even though the compiler might change the loop into something that includes references referring to particular elements, the analysis will see that none of these are potentially capturing. As a result, it could consider the array
a
dead at the time thenew MyClass
is constructed and then it could remove a copy.Motivating example for ownership transfer from non-nil owned values
Option 1 does not help this case and so it would be a compilation error, since
x: owned MyClass
is non-nilable but it storesnil
after the ownership transfer.Option 2 would allow for it because
init=
for owned is not considered to capture an alias.This is similar to the above case.
Further Interesting Examples
RAII Lock
Chapel doesn't have a C++-style RAII lock type and it's unclear if we will add that in the future. What would a C++-style RAII lock look like in Chapel? E.g.
I personally do not like the way that the un-lock is hidden and would prefer something more explicit. Python
with
contexts and Javasynchronized
blocks come to mind as a good precedents. See #12306 for the general direction I prefer.In any case an RAII lock can be directly supported by Option 1. For Option 2 we would need to either use a different lock strategy or use an attribute (see #7355) to communicate to the compiler that
deinit
for that type should not be moved.store(x, makeOwnedC().borrow()
This example is more challenging for these analyses and comes from #11534 (comment)
Should the temporary storing the result of
makeOwnedC()
be destroyed at point 1 or point 2?In Option 1, it would be destroyed at point 1. This program would fail to compile due to lifetime checking errors.
In Option 2, it would be destroyed at point 2, because
store
is capturing a potential alias to it.Option 2 Further Examples
This example shows how the lifetime clause interacts with the checking. Missing a lifetime clause on a function capturing into a global variable would lead to a compilation error. Once that lifetime clause is present, capturing is detected.
One might ask - what happens if
R
storesunmanaged C
?The above code could communicate to the compiler that
tmp
needs to be deinited at point 2 by:tmp
e.g.tmp;
just before point 2captureA
orcaptureB
to communicate to the compiler thattmp
should have global scopeNote that such a type is also able to be misused even with Option 1:
The situation might come up in the standard library with
string.localize()
andstring.c_str()
. Both of these return things that alias the original string. However it would be nice for the lifetime checker to find misuses of these, so 2. above would be a preferred. We would want to do that even if we chose Option 1, so I don't think it's asking a lot to do it to support Option 2.What do other languages do?
drop
d explicitly or when they reach the end of their block. Note that the Rust compiler actually reasons aboutdrop
s at runtime because a variable could be conditionally initialized. There was some discussion about eager drops based only upon compile-time information in Allow Eager Drops rust-lang/rfcs#239 but this approach was not chosen. The reasons were that it interferes with RAII and that it can make writing unsafe code harder. These tradeoffs were not seen as "worth it" when the only benefit described was removing the need for some stack-allocated drop flags.The text was updated successfully, but these errors were encountered: