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.

Sign up for GitHub

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

Jump to bottom

Compiler-generated initializers do not copy domain formals when assigning to fields #16290

Closed
e-kayrakli opened this issue Aug 25, 2020 · 1 comment · Fixed by #16180
Closed

Compiler-generated initializers do not copy domain formals when assigning to fields #16290

e-kayrakli opened this issue Aug 25, 2020 · 1 comment · Fixed by #16180
Labels
area: Compiler type: Bug

Comments

@e-kayrakli
Copy link
Contributor

e-kayrakli commented Aug 25, 2020 •
edited

In the code below, c.D = {1..5} assignment changes the domain of arr. We
expect that to be a constant domain.

  class MyClass {
    var D;
    var A: [D] int;

    /*
    // uncomment this to get the desired behavior
    proc init(d) {
      this.D = d;
    }
    */
  }

  var arr = [1,2,3];
  var c = new MyClass(arr.domain);
  c.D = {1..5};

  writeln("These should be different:");
  writeln(c.A.domain);
  writeln(arr.domain);

Some notes:

Future:test/classes/initializers/compilerGenerated/copyDomainFormal.chpl
e-kayrakli added a commit to e-kayrakli/chapel that referenced this issue Aug 25, 2020
@e-kayrakli
Add a future for chapel-lang#16290, notest a relevant test
c66443f
@e-kayrakli e-kayrakli mentioned this issue Aug 25, 2020
Merged
e-kayrakli added a commit that referenced this issue Aug 25, 2020
@e-kayrakli
Merge pull request #16291 from e-kayrakli/comp-init-domain-copy-future
6d85de7 
Add a future for #16290, notest a relevant test

Add a `.notest` for

  `classes/initializers/compilerGenerated/modifyDomain-array-created-by-init`

because #16218 exposed a bug that
causes this test to fail under valgrind. This is captured in
#16290.

Note that we already have 

  `classes/initializers/phase1/modifyDom-init-noType2`

which is a very similar test that contains the workaround for the bug already.

Also add a future for that issue.

[Trivial, not reviewed]
This was referenced Aug 26, 2020
Merged
Closed
mppf added a commit to mppf/chapel that referenced this issue Aug 31, 2020
@mppf
Un-future / Un-no-test tests as chapel-lang#16290 is resolved
b9cdf5f
mppf added a commit to mppf/chapel that referenced this issue Aug 31, 2020
@mppf
Un-future / Un-no-test tests as chapel-lang#16290 is resolved
984e6eb
@bradcray
Copy link
Member

For those who skip past most GitHub-generated notes and don't have access to our chat, @mppf is working on a branch that should resolve this (fingers happily crossed).

mppf added a commit to mppf/chapel that referenced this issue Sep 3, 2020
@mppf
Un-future / Un-no-test tests as chapel-lang#16290 is resolved
7856f90
mppf added a commit to mppf/chapel that referenced this issue Sep 3, 2020
@mppf
Un-future / Un-no-test tests as chapel-lang#16290 is resolved
4460217
mppf added a commit to mppf/chapel that referenced this issue Sep 8, 2020
@mppf
Un-future / Un-no-test tests as chapel-lang#16290 is resolved
0252bf3
mppf added a commit that referenced this issue Sep 8, 2020
@mppf
Merge pull request #16180 from mppf/inout-single-argument
0dddd26 
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
Assignees
No one assigned
Labels
area: Compiler type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adjust inout to be implemented with a single argument mppf/chapel
2 participants
@e-kayrakli @bradcray