Skip to content
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

array.domain and in intent #16195

Closed
mppf opened this issue Aug 4, 2020 · 5 comments · Fixed by #16180
Closed

array.domain and in intent #16195

mppf opened this issue Aug 4, 2020 · 5 comments · Fixed by #16180

Comments

@mppf
Copy link
Member

mppf commented Aug 4, 2020

What should this program do?

var D = {1..10};
var A:[D] int;

proc f(in dom) {
  D = {1..3};
  writeln("D is ", D);
  writeln("dom is ", dom);
}

writeln("passing D");
D = {1..10};
f(D);

writeln("passing A.domain");
D = {1..10};
f(A.domain);

It currently prints out

passing D
D is {1..3}
dom is {1..10}
passing A.domain
D is {1..3}
dom is {1..3}

Should it print out

passing D
D is {1..3}
dom is {1..10}
passing A.domain
D is {1..3}
dom is {1..10}

?

@mppf mppf changed the title domain and in intent A.domain and in intent Aug 4, 2020
@mppf mppf changed the title A.domain and in intent array.domain and in intent Aug 4, 2020
@mppf
Copy link
Member Author

mppf commented Aug 4, 2020

The A.domain support is currently implemented with a runtime flag. In contrast, for most other types this is a compile time behavior. Could we migrate the A.domain functionality to be compile-time as well? That would mean that A.domain would return some sort of "domain view" type. It's unclear to me in what cases having it compile-time vs. runtime would matter. Of course, a difference in the type would be observable.

@bradcray
Copy link
Member

bradcray commented Aug 4, 2020

Yeah, I think it should definitely print out the second behavior. Would it not be sufficient to have _array.domain return a const ref to the _domain record representing the array's domain? (rather than introducing the notion of a domain view).

@mppf
Copy link
Member Author

mppf commented Aug 4, 2020

We could have _array.domain return a const ref to the _domain record but for the fact that it doesn't already exist because the _array record doesn't have a _domain record - rather it has an _instance that is an array implementation class that in turn has an instance of a domain implementation class.

I have been thinking it might be possible for the compiler to mark _getDomain with pragma "no copy return" and then add compiler logic to make sure something like return A.domain results in a copy. (Also pragma "fn returns iterator").

These questions are coming from a desire to get rid of chpl__unref and chpl__unalias if possible. I am thinking it would be possible to detect cases where an initCopy returns a different type and just leave the initCopy present in that case.

@mppf
Copy link
Member Author

mppf commented Aug 6, 2020

I have been thinking it might be possible for the compiler to mark _getDomain with pragma "no copy return" and then add compiler logic to make sure something like return A.domain results in a copy. (Also pragma "fn returns iterator").

There is one case when this might have an unintended effect. Something like this:

var x = if option then A.domain else {1..100};

If we rely on the compile-time only approach, then this will result in a copy even for the case of option==false. The reason is the compiler will not know which branch is taken. It is possible that we could resolve this by adjusting the implementation of if-expressions. (Namely, the if-expression could do the copying inside of the conditional - this is what we do for returns in conditionals).

I think this is acceptable for now because we haven't yet tackled trying to get the copies involved in a domain (really, associative domain) optimized as we have for arrays (https://github.com/Cray/chapel-private/issues/962 covers that TODO). And, I would really like to remove the runtime handling in chpl__unalias and the domains are the only thing that currently use it.

@bradcray
Copy link
Member

bradcray commented Aug 6, 2020

That seems like a reasonable tradeoff to me.

mppf added a commit to mppf/chapel that referenced this issue Sep 3, 2020
mppf added a commit to mppf/chapel that referenced this issue Sep 8, 2020
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants