Add/improve errors for .localAccess() calls to non-local elements (#…

…25754)

[reviewed by @e-kayrakli ]

This PR adds and improves errors for `.localAccess()` calls that access
remote elements.

The original motivation for the PR was to fix the case noted in issue
#25747 in which `.localAccess()` on a default rectangular array would
always pass, whether that array was stored locally or on a remote locale
(captured in `test/arrays/locality/localAccess/localAccess.chpl`). It
also improves the error message for `.localAccess()` violations on block
arrays by expressing the error as a locality error rather than a simple
out-of-bounds error, as before (also reported in #25747, captured in
`test/arrays/locality/localAccess/localAccess-block*.chpl`). These new
tests make sure that these errors are generated for read and write cases
as well as for POD vs. non-POD arrays, since our modules have distinct
dsiAccess overloads for these cases.

The check itself is implemented as an additional/optional check in
`checkAccess()` which is called for all array accesses when checking is
on. It now takes an optional `ensureLocal` param that is false by
default, but set to true for all `.localAccess()` code paths to enable
the bounds checking. The logic itself generates different error messages
depending on whether the array is completely local to a single (remote)
locale, distributed but the current locale owns nothing, or distributed
but the index is outside the current locale's bounds.

The implementation approach I originally pursued to determine whether an
access was in-bounds was to use the array's `localSubdomains()` call to
check that the localAccess's indices were in-bounds. However, this
caused problems for the Stencil distribution since its localSubdomain(s)
(correctly) only refer to the indices that the locale truly owns, but
not ones that are part of its "fluff" (cached ghost cells). Yet, we also
want that fluff to be accessible using localAccess() since that's how we
get performance when the compiler can't optimize them automatically,
requiring a slightly more sophisticated approach described in the next
paragraph. New test `test/distributions/stencil/localAccess.chpl` locks
in a very simple case of this Stencil distribution pattern, with some
accesses passing due to true ownership, some due to fluff ownership, and
one generating an error due to a remote access.

The Stencil distribution challenge led to the approach taken here, which
is to introduce a new internal iterator called
`chpl__localStoredSubdomains` whose purpose is to yield a locale's
subdomains, including indices that it doesn't truly own, like the fluff
of the stencil distribution. This is implemented using an optional call
on the array, `doiLocalStoredSubdomains()`, and if not supported, we
fall back to using the `localSubdomains()` call instead. I then
implemented `doiLocalStoredSubdomains()` on Stencil distributions.

Along the way, I found that `dsiLocalSubdomains()` calls weren't
implemented for Block, Cyclic, and Stencil, so added those as well.

Then, while testing this, I found that our current tests that lock in
support for using oversubscribed block/cyclic/stencil distributions
(that is, ones in which a single locale appears in targetLocales
multiple times) were getting away with using `.localAccess()` to access
elements that weren't actually local, so I updated them to only update
local array elements. The reason for this is that the `dsiAccess()`
routine on these distributions simply falls back to a normal
`dsiAccess()` in the event of oversubscription—meaning that we're also
losing some performance there in addition to not actually doing a local
access... I also found that the stencil variant of this test was never
set up to run with multiple locales, so updated it to do so.

Note that much of the code in this effort may not be optimal
performance-wise (e.g., iterating over local subdomains when the common
case is that a locale only has a single subdomain), but that this seemed
acceptable—at least for the time being—since this code is only called
when checks are on.

Two bad files `test/expressions/if-expr/*.bad` had to be updated due to
shifting IDs in the AST… :(

Resolves #25747.

modules/dists/BlockDist.chpl

@@ -1917,6 +1917,12 @@ proc BlockImpl.chpl__locToLocIdx(loc: locale) {
   return (false, targetLocDom.first);
 }
 
+iter BlockImpl.chpl__locToLocIdxs(loc: locale) {
+  for locIdx in targetLocDom do
+    if (targetLocales[locIdx] == loc) then
+      yield locIdx;
+}
+
 // Block subdomains are continuous
 
 proc BlockArr.dsiHasSingleLocalSubdomain() param do return !allowDuplicateTargetLocales;
@@ -1953,6 +1959,17 @@ proc BlockDom.dsiLocalSubdomain(loc: locale) {
     return d;
   }
 }
+iter BlockDom.dsiLocalSubdomains(loc: locale) {
+  for locid in dist.chpl__locToLocIdxs(loc) {
+    var inds = chpl__computeBlock(locid, dist.targetLocDom, dist.boundingBox, dist.boundingBox.dims());
+    yield whole[(...inds)];
+  }
+}
+iter BlockArr.dsiLocalSubdomains(loc: locale) {
+  for subdoms in dom.dsiLocalSubdomains(loc) {
+    yield subdoms;
+  }
+}
 
 proc BlockDom.numRemoteElems(viewDom, rlo, rid) {
   // NOTE: Not bothering to check to see if rid+1, length, or rlo-1 used

modules/dists/CyclicDist.chpl

@@ -483,6 +483,12 @@ proc CyclicImpl.chpl__locToLocIdx(loc: locale) {
   return (false, targetLocDom.first);
 }
 
+iter CyclicImpl.chpl__locToLocIdxs(loc: locale) {
+  for locIdx in targetLocDom do
+    if (targetLocs[locIdx] == loc) then
+      yield locIdx;
+}
+
 proc CyclicImpl.getChunk(inds, locid) {
   const chunk = locDist(locid).myChunk((...inds.getIndices()));
   return chunk;
@@ -1621,6 +1627,17 @@ proc CyclicDom.dsiLocalSubdomain(loc: locale) {
     return d;
   }
 }
+iter CyclicDom.dsiLocalSubdomains(loc: locale) {
+  for locid in dist.chpl__locToLocIdxs(loc) {
+    var inds = chpl__computeCyclic(idxType, locid, dist.targetLocDom.dims(), dist.startIdx);
+    yield whole[(...inds)];
+  }
+}
+iter CyclicArr.dsiLocalSubdomains(loc: locale) {
+  for subdoms in dom.dsiLocalSubdomains(loc) {
+    yield subdoms;
+  }
+}
 
 proc CyclicArr.canDoOptimizedSwap(other) {
   var domsMatch = true;

modules/dists/StencilDist.chpl

@@ -2313,6 +2313,12 @@ proc StencilImpl.chpl__locToLocIdx(loc: locale) {
   return (false, targetLocDom.first);
 }
 
+iter StencilImpl.chpl__locToLocIdxs(loc: locale) {
+  for locIdx in targetLocDom do
+    if (targetLocales[locIdx] == loc) then
+      yield locIdx;
+}
+
 // Stencil subdomains are continuous
 
 proc StencilArr.dsiHasSingleLocalSubdomain() param do return !allowDuplicateTargetLocales;
@@ -2346,6 +2352,36 @@ proc StencilDom.dsiLocalSubdomain(loc: locale) {
     return d;
   }
 }
+iter StencilDom.dsiLocalSubdomains(loc: locale) {
+  for locid in dist.chpl__locToLocIdxs(loc) {
+    var inds = chpl__computeBlock(locid, dist.targetLocDom, dist.boundingBox, dist.boundingBox.dims());
+    yield whole[(...inds)];
+  }
+}
+iter StencilArr.dsiLocalSubdomains(loc: locale) {
+  for subdoms in dom.dsiLocalSubdomains(loc) {
+    yield subdoms;
+  }
+}
+
+// These are similar to the normal dsiLocalSubdomains() itertors, but
+// they yield the local blocks as extended by the fluff for the sake
+// of locality-checking rather than just the local blocks.
+//
+iter StencilDom.doiLocalStoredSubdomains() {
+  for locid in dist.chpl__locToLocIdxs(here) {
+    // Return the locally stored domain associated with each 'locid'.
+    // This shouldn't result in communication since we're always
+    // making queries about 'here' rather than a potentially remote
+    // locale.
+    yield locDoms[locid].myFluff;
+  }
+}
+iter StencilArr.doiLocalStoredSubdomains() {
+  for subdom in dom.doiLocalStoredSubdomains() do {
+    yield subdom;
+  }
+}
 
 proc StencilDom.numRemoteElems(viewDom, rlo, rid) {
   // NOTE: Not bothering to check to see if rid+1, length, or rlo-1 used

modules/internal/ChapelArray.chpl

@@ -848,7 +848,7 @@ module ChapelArray {
     pragma "insert line file info"
     pragma "always propagate line file info"
     @chpldoc.nodoc
-    proc checkAccess(indices, value) {
+    proc checkAccess(indices, value, param ensureLocal=false) {
       if this.isRectangular() {
         if !value.dsiBoundsCheck(indices) {
           if rank == 1 {
@@ -882,6 +882,29 @@ module ChapelArray {
                  "note: ", dimstr);
           }
         }
+        if ensureLocal {
+          const locInds = this.chpl__localStoredSubdomains(),
+                locIdx = || reduce for blk in locInds do blk.contains(indices);
+
+          if !locIdx {
+            if chpl_isNonDistributedArray() {
+              halt("Cannot use .localAccess() from locale ", here.id,
+                   " on a remote array stored on locale ", value.locale.id);
+            } else {
+              if locInds.size == 1 && locInds[0].size == 0 {
+                halt("Call to .localAccess() from locale ", here.id,
+                     " is illegal because it has no local array elements");
+              } else {
+                halt("Call to .localAccess",
+                     if indices.size == 1 then "(" + indices(0):string + ")"
+                                          else indices,
+                     " from locale ", here.id,
+                     " refers to remote data (local indices are: ", locInds,
+                     ")");
+              }
+            }
+          }
+        }
       }
     }
 
@@ -1031,7 +1054,7 @@ module ChapelArray {
     {
       const value = _value;
       if boundsChecking then
-        checkAccess(i, value=value);
+        checkAccess(i, value=value, ensureLocal=true);
 
       if logAllArrEltAccess ||
         (logDistArrEltAccess && !chpl_isNonDistributedArray()) then
@@ -1052,7 +1075,7 @@ module ChapelArray {
     {
       const value = _value;
       if boundsChecking then
-        checkAccess(i, value=value);
+        checkAccess(i, value=value, ensureLocal=true);
 
       if logAllArrEltAccess ||
         (logDistArrEltAccess && !chpl_isNonDistributedArray()) then
@@ -1072,7 +1095,7 @@ module ChapelArray {
     {
       const value = _value;
       if boundsChecking then
-        checkAccess(i, value=value);
+        checkAccess(i, value=value, ensureLocal=true);
 
       if logAllArrEltAccess ||
         (logDistArrEltAccess && !chpl_isNonDistributedArray()) then
@@ -1615,6 +1638,24 @@ module ChapelArray {
       }
     }
 
+    // This internal iterator is like 'localSubdomains()' but gives a
+    // distribution the option of indicating that it stores more
+    // indices than just those in its local subdomain; The stencil
+    // distribution is such an example since each locale stores cached
+    // boundary values ("fluff") in addition to its true local
+    // subdomain.  Thus, locality checks should pass if they're within
+    // the fluff even though that's outside of the normal
+    // localSubdomain() index set.
+    @chpldoc.nodoc
+    iter chpl__localStoredSubdomains() {
+      use Reflection;
+      if canResolveMethod(_value, "doiLocalStoredSubdomains") {
+        for d in _value.doiLocalStoredSubdomains() do yield d;
+      } else {
+        for d in localSubdomains() do yield d;
+      }
+    }
+
     proc chpl__isDense1DArray() param {
       return this.isRectangular() &&
              this.rank == 1 &&

test/arrays/locality/localAccess.skipif

@@ -0,0 +1,2 @@
+CHPL_COMM==none
+COMPOPTS <= --no-local

test/arrays/locality/localAccess/COMPOPTS

@@ -0,0 +1,2 @@
+-sPOD=true --checks
+-sPOD=false --checks

test/arrays/locality/localAccess/NUMLOCALES

@@ -0,0 +1 @@
+2

test/arrays/locality/localAccess/localAccess-block.chpl

@@ -0,0 +1,29 @@
+use BlockDist;
+config param POD = true;
+config const read = true;
+
+var Arr = createArray();
+
+on Locales[numLocales-1] {
+  local { if read then writeln(Arr.localAccess[0]); else Arr.localAccess[0] += 1; }
+}
+
+proc createArray() {
+  const D = {0..2} dmapped new blockDist({0..99});
+  var A: [D] if POD then int else R;
+  if POD then
+    A = [1, 2, 3];
+  else
+    A = [new R(1), new R(2), new R(3)];
+  return A;
+}
+
+record R {
+  var x: int;
+
+  operator +(ref lhs: R, rhs: int) {
+    lhs.x += rhs;
+    return lhs;
+  }
+}
+

test/arrays/locality/localAccess/localAccess-block.execopts

@@ -0,0 +1,2 @@
+--read=true
+--read=false

test/arrays/locality/localAccess/localAccess-block.good

@@ -0,0 +1 @@
+localAccess-block.chpl:8: error: halt reached - Call to .localAccess() from locale 1 is illegal because it has no local array elements

test/arrays/locality/localAccess/localAccess-block2-2D.chpl

@@ -0,0 +1,29 @@
+use BlockDist;
+config param POD = true;
+config const read = true;
+
+var Arr = createArray();
+
+on Locales[numLocales-1] {
+  local { if read then writeln(Arr.localAccess[0,0]); else Arr.localAccess[0,0] += 1;  }
+}
+
+proc createArray() {
+  const D = {0..99 by 50, 0..99 by 50} dmapped new blockDist({0..99, 0..99});
+  var A: [D] if POD then int else R;
+  if POD then
+    A = reshape([1, 2, 3, 4], A.domain);
+  else
+    A = reshape([new R(1), new R(2), new R(3), new R(4)], A.domain);
+  return A;
+}
+
+record R {
+  var x: int;
+
+  operator +(ref lhs: R, rhs: int) {
+    lhs.x += rhs;
+    return lhs;
+  }
+}
+

test/arrays/locality/localAccess/localAccess-block2-2D.execopts

@@ -0,0 +1,2 @@
+--read=true
+--read=false

test/arrays/locality/localAccess/localAccess-block2-2D.good

@@ -0,0 +1 @@
+localAccess-block2-2D.chpl:8: error: halt reached - Call to .localAccess(0, 0) from locale 1 refers to remote data (local indices are: {50..99 by 50, 0..99 by 50})

test/arrays/locality/localAccess/localAccess-block2.chpl

@@ -0,0 +1,29 @@
+use BlockDist;
+config param POD = true;
+config const read = true;
+
+var Arr = createArray();
+
+on Locales[numLocales-1] {
+  local { if read then writeln(Arr.localAccess[0]); else Arr.localAccess[0] += 1;}
+}
+
+proc createArray() {
+  const D = {0..99 by 34} dmapped new blockDist({0..99});
+  var A: [D] if POD then int else R;
+  if POD then
+    A = [1, 2, 3];
+  else
+    A = [new R(1), new R(2), new R(3)];
+  return A;
+}
+
+record R {
+  var x: int;
+
+  operator +(ref lhs: R, rhs: int) {
+    lhs.x += rhs;
+    return lhs;
+  }
+}
+

test/arrays/locality/localAccess/localAccess-block2.execopts

@@ -0,0 +1,2 @@
+--read=true
+--read=false

test/arrays/locality/localAccess/localAccess-block2.good

@@ -0,0 +1 @@
+localAccess-block2.chpl:8: error: halt reached - Call to .localAccess(0) from locale 1 refers to remote data (local indices are: {50..99 by 34 align 0})

test/arrays/locality/localAccess/localAccess.chpl

@@ -0,0 +1,26 @@
+config param POD = true;
+
+config const read = true;
+
+var Arr = createArray();
+
+on Locales[numLocales-1] {
+  if read then writeln(Arr.localAccess[0]); else Arr.localAccess[0] += 1;
+}
+
+proc createArray() {
+  if POD then
+    return [1, 2, 3];
+  else
+    return [new R(1), new R(2), new R(3)];
+}
+
+record R {
+  var x: int;
+
+  operator +(ref lhs: R, rhs: int) {
+    lhs.x += rhs;
+    return lhs;
+  }
+}
+

test/arrays/locality/localAccess/localAccess.execopts

@@ -0,0 +1,2 @@
+--read=true
+--read=false

test/arrays/locality/localAccess/localAccess.good

@@ -0,0 +1 @@
+localAccess.chpl:8: error: halt reached - Cannot use .localAccess() from locale 1 on a remote array stored on locale 0

test/distributions/block/duplicateTargetLocales.allow.comm-none.good

@@ -0,0 +1,4 @@
+1 0 0 0 0 0 0 0 0 10
+1 0 0 0 0 0 7 0 0 0
+1 3 6 10 15 21 28 36 45 55
+1 2 3 4 5 6 7 8 9 -10

test/distributions/block/duplicateTargetLocales.allow.good

@@ -1,3 +1,4 @@
 1 0 0 0 0 0 0 0 0 10
-1 0 0 0 0 0 0 0 0 10
+1 0 0 0 0 0 7 0 0 0
 1 3 6 10 15 21 28 36 45 55
+duplicateTargetLocales.chpl:26: error: halt reached - Call to .localAccess(10) from locale 0 refers to remote data (local indices are: {1..2} {6..7})

test/distributions/block/duplicateTargetLocales.chpl

@@ -16,9 +16,12 @@ writeln(A);
 
 A = 0;
 A.localAccess[1] = 1;
-A.localAccess[n] = n;
+A.localAccess[7] = 7;
 writeln(A);
 
 A = space;
 const B = + scan A;
 writeln(B);
+
+A.localAccess[n] = -n;
+writeln(A);

test/distributions/block/duplicateTargetLocales.compopts

@@ -1,2 +1,2 @@
--sBlockDist.allowDuplicateTargetLocales=false # duplicateTargetLocales.noallow.good
--sBlockDist.allowDuplicateTargetLocales       # duplicateTargetLocales.allow.good
+--checks -sBlockDist.allowDuplicateTargetLocales=false # duplicateTargetLocales.noallow.good
+--checks -sBlockDist.allowDuplicateTargetLocales       # duplicateTargetLocales.allow.good

test/distributions/cyclic/duplicateTargetLocales.allow.comm-none.good

@@ -0,0 +1,5 @@
+duplicateTargetLocales.chpl:23: warning: scan has been serialized (see issue #12482)
+1 0 0 0 0 0 0 0 0 10
+1 0 0 0 0 0 0 0 9 0
+1 3 6 10 15 21 28 36 45 55
+1 2 3 4 5 6 7 8 9 -10

test/distributions/cyclic/duplicateTargetLocales.allow.good

@@ -1,4 +1,5 @@
 duplicateTargetLocales.chpl:23: warning: scan has been serialized (see issue #12482)
 1 0 0 0 0 0 0 0 0 10
-1 0 0 0 0 0 0 0 0 10
+1 0 0 0 0 0 0 0 9 0
 1 3 6 10 15 21 28 36 45 55
+duplicateTargetLocales.chpl:26: error: halt reached - Call to .localAccess(10) from locale 0 refers to remote data (local indices are: {1..10 by 8} {1..10 by 8 align 5})