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

Promoted expression cannot be passed as array argument #16007

Closed
rahulghangas opened this issue Jul 3, 2020 · 11 comments · Fixed by #16180
Closed

Promoted expression cannot be passed as array argument #16007

rahulghangas opened this issue Jul 3, 2020 · 11 comments · Fixed by #16180

Comments

@rahulghangas
Copy link
Member

Summary of Problem

As title

Steps to Reproduce

Source Code:

proc foo(a : [] int) {
  writeln(a);
}

var a : [1..10] int = 1;
var b : [1..10] int = 2;
foo(a+b);

results in a compiler error

promotedArrayAsArgument.chpl:7: error: unresolved call 'foo(promoted expression)'
promotedArrayAsArgument.chpl:1: note: this candidate did not match: foo(a: [] int) [207314]
promotedArrayAsArgument.chpl:7: note: because call actual argument #1 with type _ir_chpl_promo1_+
promotedArrayAsArgument.chpl:1: note: is passed to formal 'a: []'
promotedArrayAsArgument.chpl:7: note: unresolved call had id 207360

Configuration Information

  • Output of chpl --version: 1.23.0 pre-release (8366399)
CHPL_TARGET_PLATFORM: darwin
CHPL_TARGET_COMPILER: clang
CHPL_TARGET_ARCH: x86_64
CHPL_TARGET_CPU: native
CHPL_LOCALE_MODEL: flat
CHPL_COMM: none
CHPL_TASKS: qthreads
CHPL_LAUNCHER: none
CHPL_TIMERS: generic
CHPL_UNWIND: none
CHPL_MEM: jemalloc
CHPL_ATOMICS: cstdlib
CHPL_GMP: gmp
CHPL_HWLOC: hwloc
CHPL_REGEXP: re2
CHPL_LLVM: llvm *
CHPL_AUX_FILESYS: none
@rahulghangas
Copy link
Member Author

Nvm, duplicate of #14364

@mppf
Copy link
Member

mppf commented Jul 6, 2020

I don't think that this is exactly a duplicate of #14364 because I would assume (and correct me if I am wrong) that in filing this you were thinking that one should be able to pass a promoted expression to an array argument; and in that event the compiler would capture the promoted expression into an array. And that it should do this with the code you have presented. In more compiler-developer terms I would say this is a request that the compiler "allow coercions from iterators to arrays with the same element type". (One wrinkle there is - what if the array argument indicated the shape? What if it were 2D? Does the iterator have to have the same shape as the array?)

That is different from #14364 because #14364 proposes some kind of short way of indicating that in the code the iterator should be converted to an array. That is, the user writes a function call, method call, or cast.

@bradcray
Copy link
Member

bradcray commented Jul 6, 2020

W.r.t. Michael's interpretation of this issue's ask, I feel nervous about the notion of having an iterator expression implicitly get converted to an array primarily because arrays are passed by ref by default and having a ref to a compiler-introduced temporary array storing the result of a+b makes me feel nervous. If foo() explicitly took a by const ref intent then I'd be less hesitant, but still a bit. If it took it by in intent, then I'd have a harder time coming up with a solid reservation given that we support var c: [1..10] int = a + b; (and I think would ultimately like to support var c: [] int = a + b;).

@mppf
Copy link
Member

mppf commented Jul 6, 2020

Yes, generally speaking we only allow coercion when passing an argument to in intent (and not for ref or even const ref).

@damianmoz
Copy link

I concur with @bradcray. In fact I just tried to pass a promoted expression to a function I wrote with a const in argument which was an array of reals. The compiler complained. It would be nice for that to work, but happy if ref and const ref do not (emphatically so for ref).

That said, if there is a way of casting that promoted expression into an array as @bradcray promotes (ha-ha) as a solution to #14364, then that approach would also solve this problem (I think). And then this coercion issue would go away as far as I am concerned. I think banning coercion of any type and demanding that a program cast the expression is more rigorous in my humble opinion.

I thought that this and #14364 were the same until I realized, as @mppf alluded to, one was casting and the other coercion.

By the way, I cannot find the words "promoted expression" anyway in the documentation as a pair of adjacent words. I had to go to the list to get some help on my problem with was this issue. No internet returned a match either. The only place I did notice the use of that term was in a recent reply to myself on the list from @bradcray but that was about another issue and I did not make the connection.

@bradcray
Copy link
Member

bradcray commented Aug 4, 2020

I think banning coercion of any type and demanding that a program cast the expression is more rigorous in my humble opinion.

@damianmoz: Just to make sure I'm understanding: You're using "more rigorous" as a positive attribute here, is that right? (i.e., you'd prefer people be forced to add the cast than for a coercion to take place automatically and invisibly?)

@damianmoz
Copy link

Yes, "more rigorous" is always a positive attribute.

@mppf
Copy link
Member

mppf commented Aug 5, 2020

In the process of resolving some other issues, (PR #16180 and issues #16185 and #16195), I think that I will end up also allowing iterators to be passed to in / const in formals accepting a type that would result from var A = someIteratorExpression. It sounded like this is probably OK but it would be good to know if there are objections to it.

@bradcray
Copy link
Member

bradcray commented Aug 5, 2020

Sounds good to me!

@damianmoz
Copy link

Remind me (again), or point me at, the latest thoughts on how to cast the promoted expression to be an array please?

In particular, consider the scenario where the return statement within some function (returning any array) casts the said expression to be an array, or that return statement returns simply the name of the array within that function. If now, further consider where a call to that function then appears in the parameter list of a totally different function as-is, i.e. without anything (I assume) to cause promotion, as in

totallyDifferentFunction(..., functionReturningArray(....), ....);

Can I then further assume that there is no need for another cast in that parameter list?

Obviously if I multiplied the results of that function result as in

totallyDifferentFunction(..., functionReturningArray(....) * 2.0, ....);

to create a new expression within that parameter list, that upon that newly promoted expression will there need to be done further casting to an array.

Am I assuming too much or have I missed something? Sorry - too many passive tense verbs in those last two paragraphs.

Note that the argument in the parameter list of totallyDifferentFunction that corresponds to that array would have a type of const ref.

@bradcray
Copy link
Member

bradcray commented Aug 6, 2020

Hi Damian —

As far as I know, we don't currently support casts from promoted expressions or iterator expressions to arrays... but I think we should (or at least, don't know of a reason that we shouldn't). Specifically, given:

var A = [1.0, 2.0, 3.0];

proc foo(X: [] ) {
  writeln("X is: ", X);
}

I'd expect that a user should be able to write things like:

foo((A * 2):A.type);
foo((A * 2):[0..2] real);
foo((A * 2):[A.domain] A.eltType);

or potentially even:

foo((A * 2):[] real);                                                         
foo((A * 2):[]);

yet none of these work today. If you wanted this capability, one way to do it would be to write an explicit function that "does the cast" where one way to write it using only user-facing features, but in a fragile manner, would be the following:

proc toArray(ir) {
  var arr = ir;
  return ir;
}

The reason I say "fragile" is that, as written, this function could take any type, and not all of them would turn into arrays. A way to make this robust, but to lean on features that we haven't yet exposed to users would be to specify the type of the ir argument:

proc toArray(ir: _iteratorRecord) {
  var arr = ir;
  return ir;
}

at which point, it's possible to write:

foo(toArray(A * 2));

But since we've now dipped our toes into features that aren't user-facing, you could also write the cast yourself (which also isn't a user-facing feature yet), as follows:

proc _cast(type t:_array(?), ir: _iteratorRecord) {
  var arr = ir;
  return ir;
}

at which point these versions work:

foo((A * 2):A.type);
foo((A * 2):[0..2] real);
foo((A * 2):[A.domain] A.eltType);

Note that both of these approaches (cast or toArray utility function) have their advantages. The cast permits you to say what indices the array should have, but also makes you talk about those indices (and the array's element type) pretty explicitly. Whereas the toArray function lets you just say "make this thing an array and let's not discuss the details" and you'll get an array back (with default indices). You could also imagine other solutions in which the toArray() routine established a different lower bound or took the lower bound in as an argument or ...

None of this is specific to your case of "applying a promoted operator to a returned array", but I think these features will work fine in that context as well, and that it's just a matter of chaining expressions.

If the cast shown above is as correct and appropriate as I think it is, it may be that we should just drop that into the internal modules and have it be available by default.

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.

4 participants