Make observable inout copy/writeback/deinit orders more consistent #16143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mppf
changed the title
This was referenced Jul 27, 2020
and in that case, never set it to a reference type
This PR adjusts `inout` intent handling to handle the `in` and `out` parts separately: * the `in` part happens at the call site, as with `in` intent * the `out` part happens inside the body of the function To get foo2 more consistent in inout-intent-order.chpl
By fixing the next inout actual before handling in intents, we can avoid the need for the map.
See e.g. test/arrays/deitz/part7/test_out_array.chpl
See e.g. test/types/string/sungeun/c_string/intents.chpl
See library/standard/Set/types/testTuple.chpl In lifetime.cpp, GatherTempsVisitor::enterCallExpr does not consider PRIM_ASSIGN. It probably should but that is left as future work.
it will be a ref formal after resolve intents but it doesn't need to be a ref formal during resolution.
We had special code adding default arguments to the call that the promotion wrapper will use. But the default arguments will be handled when that call is resolved later anyway.
(otherwise, would have to update canCoerce etc logic) See test/classes/vass/ref-like-intents/inout-subclass.chpl
vasslitvinov
approved these changes
Jul 30, 2020
3 tasks
mppf
added a commit
that referenced
this pull request
Sep 8, 2020
Adjust inout to be implemented with a single argument Resolves #16290 Resolves #16185 Resolves #16195 Resolves #16148 Resolves #16275 Resolves #16301 Resolves #16298 Resolves #16300 Resolves #16007 This PR takes the following steps: * changes `inout` to use a single argument instead of two arguments * removes `chpl__unref` and instead the compiler treats types that have `chpl__initCopy` return a different type specially. Additionally, for domains, instead of relying on runtime information, use compiler analysis to identify functions that return unowned domains as with `A.domain`. * allows types that have `chpl__initCopy` to return a different type to coerce to that type. This allows iterators to pass to array arguments with `in` intent (requested in issue #16007). Follow-up to PR #16143 which changed `inout` to be implemented with an `in` argument and an `out` argument. In reviewing that PR, Vass pointed out that there might be alternatives to the two-argument approach. Since the copy-for-in and write-back-for-out are both now occurring at the call site, it is possible for the function body to just accept a `ref` argument for `inout`. The function will accept the result of the copy-for-in and then modify it. The code at the call site will do the write-back from that to the original variable. This PR makes that change. The implementation involved adjusting wrappers.cpp and adding a map to track the value copied from in the `in` intent part of `inout` for use in the `out` intent part. There is one inout behavior change: ``` chapel proc out_inout(out a: R, inout b: R) { writeln("a is ", a); writeln("b is ", b); a = new R(10); b = new R(20); // here } proc test_out_inout() { writeln(); writeln("test_out_inout"); var a = new R(1); var b = new R(2); out_inout(a, b); writeln(a, " ", b); } test_out_inout(); ``` This deinitializes `new R(20)` inside of the body of `out_inout` (instead of transferring that responsibility to the call site). As a result, the deinitialization order for `new R(20)` is different (20 is deinitialized before 10). The order of write-backs and other deinitializations at the call site is still the same. In the process of addressing problems with `inout` for arrays, I identified several issues related to array views and copy semantics such as #16185 and #16195. This caused me to make the copy changes described above including removing `chpl__unref`. Here is a more detailed summary of changes: * Adjusts wrappers.cpp, normalize.cpp to use a single argument for inout handling * Adds logic to `getInstantiationType` that considers if the type should be used for a value to be returned/stored in a variable/passed by in intent. This calls `getCopyTypeDuringResolution` which keeps track of resolved `initCopy` calls and the types that they return. When the result of `initCopy` produces a different type, then the type needs special handling here. In particular, for example, `proc f(in arg)` called with an array view should instantiate with a non-view array (resulting from the copy). * Adjusts canCoerce to use `getCopyTypeDuringResolution` and to allow coercion from a type to the result of `initCopy` on that type when working with an `in`/`const in`/`inout` formal argument. * Removes `chpl__unalias` and most `chpl__unref` calls; replaces these with initCopy * For domains that are "borrowed" such as `A.domain`, use simple compile-time tracking of such domains to add an `initCopy` for returning the borrowed domain or passing it to `in` intent. Adds `isCallExprTemporary` and `isTemporaryFromNoCopyReturn` to implement this analysis. * adjust insertUnrefForArrayOrTupleReturn to use initCopy instead of `chpl__unalias` and to use the domain analysis to call it for cases like `A.domain` * adjusts split init to use the domain analysis for cases like `A.domain` * makes rank change and reindex arrays include `param ownsArrInstance` to distinguish between rank change/reindex arrays that own their elements vs those that do not. Adjusted isRankChangeArrayView / isReindexArrayView to return `false` for such arrays that own their elements. * adds `PRIM_SET_ALIASING_ARRAY_ON_TYPE` and uses it in rank change / reindex array views so that the `FLAG_ALIASING_ARRAY` flag on the type is set appropriately and according to `param ownsArrInstance. * move isAliasingArrayImplType and isAliasingArrayType to type.cpp so they can be used in more than one place. * add `FLAG_NO_COPY_RETURNS_OWNED` to work around an order-of-resolution issue. * removes an old, no longer necessary, workaround for initCopy functions in updateVariableAutoDestroy. * updated `--print-module-resolution` output to print out the path of modules being resolved for additional information. This is only currently tested in the test named print-module-resolution.chpl. * removed dead code in postFoldPrimop * adjusted tuple code to avoid copying ref tuples in tuple init functions and to copy all elements in initCopy functions * adjusts `<<=` and `>>=` functions to accept an integral shift amount the way that `<<` and `>>` do. This was to work around an issue that is no longer present but seemed like an improvement. * adjusts sync/single initializers to use concrete `const ref` intent for sync/single arguments instead of `const`. This was to work around an issue that is no longer present but seems like an improvement. Reviewed by @e-kayrakli - thanks! - [x] primers pass with verify+valgrind and do not leak - [x] primers pass with gasnet - [x] full local futures testing Future work * nested call in function call returning runtime type executes twice #16316 * This PR makes some errors relating to returning tuples of owned runtime errors instead of compile-time errors. See #14942. This should be revisited after the resolution of #16233 / #15973. * #16339 about updating the spec description of how `ref` intent impacts candidate selection
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #15645.
This PR adjusts the compiler to implement support for
inoutbysplitting it into two arguments, an
inargument and anoutargument.This allows the
inpart ofinoutto interleave with otherinarguments in a natural manner and allows the
outpart ofinouttointerleave with other arguments in a natural manner.
At first, I tried to do this with one argument, but I ran into the
problems described here
#15645 (comment)
a separate argument from the one receiving a copy - which is problematic
if the copy is occuring at the call site, which it needs to in order to
interleave properly with other
inintent arguments.The implementation approach here is to add a special formal, marked with
FLAG_HIDDEN_FORMAL_INOUT, just after eachinoutformal in normalize.This special formal will handle the
outpart - and the originalinoutformal will handle the
inpart. Then, adjustResolutionCandidatetonot require that the hidden formal is passed. Then, adjust
wrapAndCleanUpActualsto handle theinpart of aninoutargument inhandleInIntent, and theoutpart inhandleDefaultArg(which passesthe original argument to the
outformal) and then inhandleOutIntents. Since theinout/inpart involves adding a tempthat obscures the original argument, the
fixHiddenInoutArg()call setsthe actual for the hidden argument appropriately before running the
inpart.
Besides adjusting
inout, this PR adjustsaddLocalCopiesAndWritebacksto process formals in declaration order to make the order of
deinitialization more consistent. Previously, they were being processed
in the order they were stored in a map. While there, adjusted this
function to add elements to AST in regular order instead of reverse
order.
Lastly, I noticed that the promotion wrapper was calling special
functions to handle default arguments - addDefaultTokensAndReorder(),
replaceDefaultTokensWithDefaults(), fixDefaultArgumentsInWrapCall(). But
the promotion wrapper function will be resolved in its entirety and when
the call to the original function is resolved the default arguments will
be handled (along with
inintent processing and lots of other stuffthat happens in wrapAndCleanUpActuals). Since default arguments will be
able to be handled at that later time, I removed the special handling of
default arguments in the promotion wrapper and the related support code.
Reviewed by @vasslitvinov - thanks!