Remove "no promotion when by ref" pragmas from array view routines

[bradcray]

In trying to understand what these pragmas were doing as part of the reshape work (#27132), Jade and I came to the conclusion that, in the case of array views, they probably aren't doing much, so removing them to avoid being needlessly superstitious.

TODO:

• beef up that description to make it more convincing

[bradcray]

@jabraham17 : No rush to get to this before you're back and settled, but following up on one of our pre-release discussions, I put this cleanup together and it seems to pass testing as expected…

[jabraham17]

Jade and I came to the conclusion that, in the case of array views, they probably aren't doing much, so removing them to avoid being needlessly superstitious.

I don't believe this is the case here. We were discussing functions which return array views which cannot be promoted to return a view.

These functions return modifiable array views

var B  = [0..1, 2..3, 4..5, 6..7, 8..9];
var A = [i in 0..100] i;

writeln(A[B[0]]);

A[B[0]] = 2;

writeln(A[B[0]]);

This is a valid program and is not affected by the changes in this PR. But, consider the slight changes for the following program

var B  = [0..1, 2..3, 4..5, 6..7, 8..9];
var A = [i in 0..100] i;

writeln(A[B]);

A[B] = 2;

writeln(A[B]);

This program does not compile today.

bar.chpl:6: error: yielding a non-reference from a const ref iterator
bar.chpl:6: error: yielding a const or not a reference from a non-const ref iterator
$CHPL_HOME/modules/internal/ChapelIteratorSupport.chpl:395: In function '_getIterator':
$CHPL_HOME/modules/internal/ChapelIteratorSupport.chpl:396: error: cannot iterate over values of type int(64)
$CHPL_HOME/modules/internal/ChapelIteratorSupport.chpl:396: note: unresolved call had id 1803424
  $CHPL_HOME/modules/internal/ChapelIteratorSupport.chpl:376: called as _getIterator(x: int(64)) from function '='
  bar.chpl:8: called as =(ic: _ir_chpl_promo1_this__ref__array_DefaultRectangularArr_1_int64_t_one_int64_t_int64_t, xs: int(64))
note: generic instantiations are underlined in the above callstack

If it should compile, then the pragma is absolutely needed here, and this PR would change the behavior of --warn-potential-races for this program. I am not yet convinced one way or another if this second snippet should compile.

[bradcray]

I see. Is it the case, then, that any procedure that returns an array by ref or has pragma "fn returns aliasing array" should trigger a --warn-potential-races warning, in which case we could check for those conditions for such array-view-returning procedures rather than manually attaching an additional pragma to them?

[jabraham17]

The specific case for that pragma is about modifying promoted expressions. So its not about an arbitrary proc which returns a ref/aliasing array, its a promoted expression that returns a ref/aliasing array, because the promoted expression has the potential to create a race through implicit parallelism

[bradcray]

Right, but my point is: doesn't any procedure that returns a reference to an existing array — whether by using a ref return intent or by using pragma "fn returns aliasing array" — subject to causing races if the procedure is promoted since it will likely result in multiple references to the same array? And if so, couldn't/shouldn't the compiler check for such cases independent of whether they have a pragma "no promotion when by ref" attached to them?

[jabraham17]

Any procedure which, when promoted, can return multiple mutable pointers (refs, aliasing arrays, etc) to the same location in memory/array can be a race and falls under --warn-potential-races.

Since the original PR/issue was only about indexing expressions, I only added that pragma to those procedures which support indexing for arrays to selectively enable the warning. But in theory, other procedures could suffer from this problem and should also be warned about. We could perhaps try removing this line

chapel/compiler/main/checks.cpp

Line 209 in 0d8e588

fn->hasFlag(FLAG_NO_PROMOTION_WHEN_BY_REF)) {

and run a paratest with --compopts --warn-potential-races to see what happens