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

Adjust inout to be implemented with a single argument #16180

Merged
merged 63 commits into from Sep 8, 2020

Conversation

mppf
Copy link
Member

@mppf mppf commented Jul 31, 2020

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 Promoted expression cannot be passed as array argument #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:

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!

  • primers pass with verify+valgrind and do not leak
  • primers pass with gasnet
  • full local futures testing

Future work

Copy link
Contributor

@e-kayrakli e-kayrakli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mostly minor comments in-line. Depending on your level of confidence, you can also ask @vass to take a look, especially at the changes in wrappers.cpp. Otherwise, looks good!


if (developer == true) {
retval = astr(retval, " [", istr(arg->id), "]");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding this

compiler/resolution/ResolutionCandidate.cpp Outdated Show resolved Hide resolved
compiler/resolution/callDestructors.cpp Show resolved Hide resolved
compiler/resolution/callDestructors.cpp Show resolved Hide resolved
compiler/resolution/callDestructors.cpp Show resolved Hide resolved

IntentTag intent = INTENT_OUT;
if (outFormal->intent == INTENT_INOUT ||
outFormal->originalIntent == INTENT_INOUT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to have a helper for checking intent and originalIntent as it repeats quite a few times

compiler/resolution/resolveFunction.cpp Show resolved Hide resolved
compiler/resolution/resolveFunction.cpp Outdated Show resolved Hide resolved
modules/internal/ChapelSyncvar.chpl Show resolved Hide resolved
All this is trying to get this test
 ./test/arrays/ferguson/semantic-examples/4-pass-slice-inout.chpl

to work
Some chpl__unref might still be called for tuples; a TODO
is to investigate if that is still the case.
* better address infinite loop issue
* fix problem with instantiation type for type variables

See e.g. primers syncsingle.chpl and chplvis4.chpl.
See e.g.
 test/arrays/bradc/arrOfDom/arrOfDom4.chpl
Because of the different way in which the initCopy for an iterator
was being resolved, certain functions were not visible. Resolved
that by adjusting fixInstantiationPointAndTryResolveBody to
consider the case of a type that is not instantiated.

Additionally, avoided recursion in resolving initCopy for an iterator
by adding two `pragma "no copy"`s.
See e.g. classes/bradc/arrayInClass/genericArrayInClass-otharrs
See e.g. test/studies/kmeans/kmeans-blc.chpl
Per discussion in issue chapel-lang#16275

Also adds the test variants from chapel-lang#16275 to views-and-copying.chpl
The analysis in noAliasSets.cpp uses the flag FLAG_ALIASING_ARRAY
to know when one array might alias another. Update rank change/reindex
array types that do not alias to remove that flag.
See e.g. test/classes/initializers/promotion/test_promote_initializer_owned.chpl
For test/expressions/loop-expr/forall-over-zip-over-arrays.chpl
I created issue chapel-lang#16329 to ask if we should allow inout/out
intents for extern/export.
For e.g. test/library/standard/DataFrames/psahabu/AddSeries.chpl
@mppf mppf merged commit 0dddd26 into chapel-lang:master Sep 8, 2020
@mppf mppf deleted the inout-single-argument branch September 8, 2020 17:57
mppf added a commit that referenced this pull request Sep 9, 2020
Fix compilation errors with rank change/reindex privatize

Follow-up to PR #16180 to fix compilation errors related to assignment 
between rank change / reindex arrays that differ in ownsArrInstance.
These errors were coming about due to privatization support code.

Trivial and not reviewed.

- [x] full local testing
- [x] `--no-local` testing
mppf added a commit that referenced this pull request Oct 5, 2020
Fix formatting near inout intent and add discussion of iterators passing to in

Follow-up to PR #16180.

* fixes a few formatting errors
* adds discussion of passing an iterator to an `in` intent argument

Reviewed by @vasslitvinov - thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment