-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
split-init interaction with warning about local-from-distributed #14746
Comments
Brainstorming some ideas to resolve this:
|
I'd probably do 2b myself. Could you save me some time to reproduce myself by pointing to where the warning would come from in [Adding more rationale for this choice:
My gut reaction is not to extend the warning to assignment (because it seems like it would make many domain-to-domain assignments annoying), but maybe that's just because I understand the language well enough that I'm not a huge fan of the warning to begin with and think people shouldn't be declaring their domain variable types so much? Though I understand that many new users have gotten pretty flummoxed by this behavior which is why we have it...] |
Sure. chapel/test/studies/hpcc/HPL/bradc/hpl-blc-noreindex.chpl Lines 186 to 189 in 921f541
|
Thanks for pointing to the code segments. I don't think either of those change my answer (and I provided a bit more rationale for my answer in an edit just above, capturing some of our offline discussion). |
I'll plan to do 2b for the moment. |
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
Include local-from-distributed warning in .goods after split init Per discussion in #14746, update .good files to include warning about initializing a non-distributed domain from a distributed one, in tests that now run into this due to split-init. The warning was added in PR #10791, the workarounds added in #14691. Test changes only; not reviewed.
Addressed (for now at least) by PR #14802 which does 2b. |
How should split init interact with the warning about initializing a non-distributed domain from a distributed one?
Previously this warning relied on the difference between initialization and assignment. The warning was added in PR #10791.
For example:
When this is resolved, update these tests, as PR #14691 adds a workaround to them:
The text was updated successfully, but these errors were encountered: