-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add/improve errors for .localAccess()
calls to non-local elements
#25754
Changes from all commits
22618dd
4128a23
f55e69d
b71e027
ef3e202
1175602
34a636e
adabcef
802c72a
9a30dd7
12f63a0
5df5a4a
266fb3f
435c1bf
f7270bd
f76bf02
24b927e
629421f
05ec799
2f9db17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
} | ||
} | ||
iter CyclicArr.dsiLocalSubdomains(loc: locale) { | ||
for subdoms in dom.dsiLocalSubdomains(loc) { | ||
yield subdoms; | ||
} | ||
} | ||
|
||
proc CyclicArr.canDoOptimizedSwap(other) { | ||
var domsMatch = true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
} | ||
} | ||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
CHPL_COMM==none | ||
COMPOPTS <= --no-local |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
-sPOD=true --checks | ||
-sPOD=false --checks |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
--read=true | ||
--read=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
--read=true | ||
--read=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
--read=true | ||
--read=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
--read=true | ||
--read=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
localAccess.chpl:8: error: halt reached - Cannot use .localAccess() from locale 1 on a remote array stored on locale 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not following the motivation for the changes in However, if I am following the diff correctly, there is no test for the error message of illegal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're correct. We were permitting illegal localAccesses in oversubscribed cases (and still do with checks off), and you're also correct that adding a test of that would be good. Perhaps I can just add it to the end of these tests. This also makes me realize: I'll need to skipif these tests with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, just stick a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, thanks for the reminder. I don't do this nearly often enough… Are you confident that compopts will follow the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've just verified that compopts is sufficient… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
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}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised by this implementation. Shouldn't it be just
yield this.locDoms[locid].myBlock;
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good question. I took this approach due to the
else
clause in thedsiLocalSubdomain()
routine just above, and to make it work with oversubscription; but in retrospect, I think you're right that I over-engineered it. Specifically, I believe I was thinking thatmyBlock
would only work in a non-oversubscribed environment, but with the loop overlocid
s, I think you're probably right. I'll check that out to make sure that it works and doesn't introduce communication.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, and I can update the tests to do the localAcesss calls within
local
blocks as a way to both verify this and lock it in going forward.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, it is necessary (if regrettable from a complexity standpoint) to do here since this routine takes a locale argument, so may be answering local subdomain queries about a remote locale's ownership, and we don't want to communicate with that remote locale to answer the question.
We could simplify / streamline the (common)
locale == here
case by either putting a conditional into this loop to check for it, or by creating two overloads of the routine, one which takes no argument (defaulting tohere
) and the other which doesn't—then updating ChapelArray.chpl's code to similarly split the implementation into two versions, each of which calls the corresponding one here. I don't feel strongly inclined to do either of those things in this PR because I worry about keeping the two implementations in sync and would need to add more testing to distinguish between the remote and local cases. It also feels a little like a change to the public interface (though it's one that I think would be acceptable since it wouldn't break existing code); or we could keep the same public interface and put a conditional into the outer call, but that doesn't seem great either.That said, the
StencilDom.doiLocalStoredSubdomains
case should not be as expensive as I was fearing in the comment you commented on now that we don't accept a locale argument in that iterator, so that comment does need to be updated (and I can add a locality check to that test to lock that in as well).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I see. I agree that the solution you outlined would be good, but shouldn't be a part of this PR.