inout varargs not yet handled correctly #16148
Comments
3 tasks
mppf
added a commit
that referenced
this issue
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
PR #16143 improves upon inout handling but leaves
inoutvarargs causing compilation error.Associated Future Test(s):
test/arrays/ferguson/inout-tuple-ref-wait-array.chpltest/arrays/ferguson/inout-tuple-ref-wait-int.chpltest/users/ferguson/bulkerr.chplThe text was updated successfully, but these errors were encountered: