Should storing an array view in a variable make it non-view? #16275
Comments
|
While we can define these any way we want, I would prefer consistency among these three. |
So far, only improves the array parts.
|
how to work on that bug guys its a very easy |
|
@kateaditya646 - I don't think this issue is easy or a good starting issue. Please see https://chapel-lang.org/contributing.html for guidance on how to contribute to this project (if you haven't already looked at it). |
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
One of the cases stopped printing out a rank change array (due to the resolution of issue chapel-lang#16275). See issue chapel-lang#16300 which asks questions about the current output.
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
One of the cases stopped printing out a rank change array (due to the resolution of issue chapel-lang#16275). See issue chapel-lang#16300 which asks questions about the current output.
|
I agree that all of the array captures / returns should be copies and not views. IIRC (and a quick check of the source suggests this), slices use the domain that was used to express the slice (and its distribution) to characterize the slice's domain, whereas rank-change slices and reindexings need to create a new domain (and distribution) in general; so it seems to me it is probably correct for them to be asymmetric in this regard. Specifically, note that the ArrayViewSlice.chpl file doesn't have any classes corresponding to distributions or domains, but the other to ArrayView files do. That's what makes me think that there's a natural/intentional asymmetry here. This suggests to me that perhaps |
I agree about that.
AFAIK they are not user facing. I was just using them as an easy way for the test to check if it was getting a view or non-view type
I think you are talking about the which seems reasonable to me. |
|
@bradcray - I am wondering what you would expect the domain type for a view-captured-into-an array to be. proc returnIt(arg) { return arg; }
proc main() {
var A: [1..2] real;
var AA: [1..2, 1..2] real;
{
writeln();
writeln("reindex");
var B = A.reindex(3..4);
writeln("to var is view: ", B.isReindexArrayView());
const ref rD = B.domain;
writeln("var.domain is view: ", rD.isReindexDomainView());
}
{
writeln();
writeln("rank change");
var B = AA[1, ..];
writeln("to var is view: ", B.isRankChangeArrayView());
const ref rD = B.domain;
writeln("var.domain is view: ", rD.isRankChangeDomainView());
}
}Based on your comments on #16298 and #16300 - I suspect that your answer might be "the rank change/reindex domain". FWIW the above program has this behavior on the main branch: and currently this behavior with PR #16180: Is the expected behavior this: ? But is this interpretation in conflict with
? I guess where I am feeling confused is that preserving the rank change domain would imply that the array is also a rank change array (because, otherwise, default rectangular (or other) array implementations would need to support rank change domains, which defeats part of the point of array views). We could certainly make a copy of the elements either way, but making a copy and leaving the array as a rank change is the original question of this issue. #16300 (comment) discusses another concern with letting |
|
Note that issue is specifically about the type of the array view arrays. The element copies (or not) are occurring as expected: proc returnIt(arg) { return arg; }
proc main() {
var A: [1..2] real;
var AA: [1..2, 1..2] real;
{
writeln();
writeln("slice");
A = 0;
const ref rB = A[1..1];
A = 1;
writeln("to const ref behaves view: ", A[1]);
A = 0;
var B = A[1..1];
A = 1;
writeln("to var behaves view: ", B[1]);
A = 0;
const ref rReturned = returnIt(A[1..1]);
A = 1;
writeln("returned behaves view: ", rReturned[1]);
}
{
writeln();
writeln("reindex");
A = 0;
const ref rB = A.reindex(3..4);
A = 1;
writeln("to const ref behaves view: ", rB[3]);
A = 0;
var B = A.reindex(3..4);
A = 1;
writeln("to var behaves view: ", B[3]);
A = 0;
const ref rReturned = returnIt(A.reindex(3..4));
A = 1;
writeln("returned behaves view: ", rReturned[3]);
}
{
writeln();
writeln("rank change");
AA = 0;
const ref rB = AA[1, ..];
AA = 1;
writeln("to const ref behaves view: ", rB[1]);
AA = 0;
var B = AA[1, ..];
AA = 1;
writeln("to var behaves view: ", B[1]);
AA = 0;
const ref rReturned = returnIt(AA[1, ..]);
AA = 1;
writeln("returned behaves view: ", rReturned[1]);
}
}prints this on master and with PR #16180: |
|
If |
Yeah, sorry. I was not clear about that. Regarding your question about the captured array not being a view, but its domain being a view, I see the conflict that you're pointing out (and suspect that this is at the heart of why we're not capturing reindex and rank-change slices on master today?). Let me stew on this a bit more; I'm worried any answer I give off the cuff will be incorrect / problematic in the distributed case. |
|
OK, I thought about this a bit more over lunch, and am now trying to capture my thoughts crisply, typing them down as I do so... (it may sound like I'm lecturing, but I'm really just trying to get the facts straight for myself, so if I'm lecturing anyone, it's myself). Given a block-distributed array: var A = newBlockArr({1..10}, real);running on a 2-locale system, we'd expect locale 0 to own ref B = A.reindex({0..9});then we'd expect locale 0 to own var C = B;we'd expect C's index set to be 0..9 and to have the same distribution as B and reindexed-A, such that locale 0 owns While for a block distribution, it's conceivable to think of creating a new block distribution and domain that describe C's mapping directly, in the general case, not all distributions can support arbitrary closed-form representations of rank-change slices or reindexings (e.g., a Cyclic 1D distribution targeting three locales would have a hard time supporting a slice of But C obviously isn't, or shouldn't be, an array view in the sense that it has its own storage and isn't referring to a different array. How does this work today, since you note a few comments above that it is? (checking...). From what I'm seeing, it looks like C is represented the same way as "reindexed A" was in the sense that What if, instead, dsiBuildArray() returned a new/distinct array type as you propose above, Taking dsiAccess() as an example, if we wrote So, if they're so similar, what would be the motivation for Or, could/should the existing calls to check whether or not something is an array view be made into non-param queries such that they can check So I think that's why we are where we are today. I'm pausing here for now to get your thoughts, @mppf. |
|
@bradcray - thanks very much for talking yourself (and me) through all of that.
Somehow I never really caught on to this part, but it's good to know what's going on.
Yep, that makes sense to me too. The compiler would just need to know how to check the param field in the right places, which is slightly harder but not a big deal.
We used to do this, but it basically adds a bunch of complexity to the compiler-modules interface and is the reason we had |
Assuming the compiler currently relies on the "is*View() routines" (does it?) I was thinking that we could change them to check the param field (or add new queries and use them instead). If the compiler is doing something more primitive like looking directly at the type, you're right of course. |
No, but this isn't a big deal either way. |
|
Anyway I think I will try the |
Per discussion in issue chapel-lang#16275 Also adds the test variants from chapel-lang#16275 to views-and-copying.chpl
|
@bradcray - what about this part? I think that we want |
|
Hmm, that's a good question. (thinking...). Yes, I believe so. D should store the indices {3..4} but using the distribution that governs A's original indices, so I think it would need to be a reindex domain to achieve that in the general case. |
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
One of the cases stopped printing out a rank change array (due to the resolution of issue chapel-lang#16275). See issue chapel-lang#16300 which asks questions about the current output.
Per discussion in issue chapel-lang#16275 Also adds the test variants from chapel-lang#16275 to views-and-copying.chpl
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
One of the cases stopped printing out a rank change array (due to the resolution of issue chapel-lang#16275). See issue chapel-lang#16300 which asks questions about the current output.
Per discussion in issue chapel-lang#16275 Also adds the test variants from chapel-lang#16275 to views-and-copying.chpl
|
PR #16180 will address these and adds the test |
So far, only improves the array parts.
For ./test/arrays/ferguson/views-and-copying.chpl
One of the cases stopped printing out a rank change array (due to the resolution of issue chapel-lang#16275). See issue chapel-lang#16300 which asks questions about the current output.
Per discussion in issue chapel-lang#16275 Also adds the test variants from chapel-lang#16275 to views-and-copying.chpl
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
(Related to #5474 and #12178).
Generally, we have the behavior that storing a slice into a (non-ref) variable causes a copy of the slice to be made. Similarly for returning a slice from a function by value. (But we never created syntax for the alternative - see #12178).
In working on #16180, I was surprised that copying a reindex/rank change array left one with a reindex/rank change array. (However it does copy the elements). I was expecting that an array view would no longer be an array view once a copy of it is made. We've had this behavior since 1.18 (and the test program below didn't compile in 1.17 or 1.16).
Here is a test program to demonstrate:
It outputs
however I would expect only the "to const ref" rows to print out
true.The text was updated successfully, but these errors were encountered: