From a9abba9c30a08562a74ac04f773662ebeeaaac8d Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 10 Jan 2025 15:30:49 -0800 Subject: [PATCH 01/15] Snapshot of fixing Set zippering Adds a pair of leader/follower iterators to ChapelHashtable and a helper method, and calls them from the Set `these` iterators. The implementation does frequent recomputation of how to divide up the iteration space to find full slots. This can probably be improved on, but I wanted to take a snapshot of what worked before attempting it. It also does not help when zippering with arrays yet. That is also a next step ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 97 +++++++++++++++++++++++++++ modules/standard/Set.chpl | 12 ++++ 2 files changed, 109 insertions(+) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 9f029f1c1cda..81ed33dce3f9 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -644,5 +644,102 @@ module ChapelHashtable { rehash(newSize); } + + proc _determineEvenChunks(numChunks: int, numFull: int): [] int { + var numFullPerTask = numFull / numChunks; + //writeln("min number of full slots per task = ", numFullPerTask); + var rem = numFull % numChunks; + //writeln("remainder = ", rem); + + var chunkEnds: [0..#(numChunks-1)] int; + var chunkIdx = 0; + var curNumFull = 0; + for i in 0..#(this.tableSize) { + if this.isSlotFull(i) { + curNumFull += 1; + } + /* If we've found the last full slot or we've filled our list of chunk + end points, exit the loop */ + if (curNumFull == numFull) || + (chunkIdx >= chunkEnds.size) { + break; + } + if chunkIdx < rem { + if (curNumFull == (numFullPerTask * (chunkIdx+1)) + (chunkIdx+1)) { + //writeln("index ", i, " is getting saved in chunkEnds[", chunkIdx, + // "]"); + chunkEnds[chunkIdx] = i; + chunkIdx += 1; + } + } else { + if (curNumFull == (numFullPerTask * (chunkIdx+1)) + rem) { + //writeln("index ", i, " is getting saved in chunkEnds[", chunkIdx, + // "]"); + chunkEnds[chunkIdx] = i; + chunkIdx += 1; + } + } + } + //writeln("chunkEnds = ", chunkEnds); + return chunkEnds; + } + + iter _evenSlots(param tag) where tag == iterKind.leader { + var numChunks = _computeNumChunks(this.tableNumFullSlots); + //writeln("number of tasks = ", numChunks); + //writeln("number of full slots = ", this.tableNumFullSlots); + + var chunkEnds = _determineEvenChunks(numChunks, this.tableNumFullSlots); + + for i in chunkEnds.domain { + if (i == 0) { + yield (0..chunkEnds[i], 0, numChunks); + } else { + yield ((chunkEnds[i-1]+1)..chunkEnds[i], i, numChunks); + if (i == chunkEnds.domain.high) { + yield ((chunkEnds[i]+1)..(this.tableSize-1), i+1, numChunks); + } + } + } + } + + iter _evenSlots(followThis, param tag) const ref + where tag == iterKind.follower { + //writeln("in follower: ", followThis); + use Types; + + var iterSpace: range; + + if (isTuple(followThis)) { + if (followThis.size == 3) { + // Using _evenSlots iterator gives us a specific set up to iterate + // over the full chunks in an even manner. + var intendedSpace = followThis(0); + var intendedChunk = followThis(1); + var numChunks = followThis(2); + + // Determine corresponding chunk to use. + var chunkEnds = _determineEvenChunks(numChunks, + this.tableNumFullSlots); + if (intendedChunk == 0) { + iterSpace = 0..chunkEnds[intendedChunk]; + } else if (intendedChunk == chunkEnds.size) { + iterSpace = (chunkEnds[intendedChunk-1]+1)..(this.tableSize - 1); + } else { + iterSpace = (chunkEnds[intendedChunk-1]+1)..chunkEnds[intendedChunk]; + } + } else { + iterSpace = followThis(0); + } + } else { + iterSpace = followThis; + } + + foreach slot in iterSpace { + if this.isSlotFull(slot) then { + yield this.table[slot].key; + } + } + } } } diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index 653a8c9db4d0..566921df1726 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -563,17 +563,29 @@ module Set { @chpldoc.nodoc iter const these(param tag) where tag == iterKind.leader { + for followThis in _htb._evenSlots(tag) { + //writeln(followThis); + yield followThis; + } + /* var space = 0..#_htb.tableSize; for followThis in space.these(tag) { yield followThis; } + */ } @chpldoc.nodoc iter const these(param tag, followThis) const ref where tag == iterKind.follower { + //writeln(this); + foreach val in _htb._evenSlots(followThis, tag) { + yield val; + } + /* foreach idx in followThis(0) do if _htb.isSlotFull(idx) then yield _htb.table[idx].key; + */ } @chpldoc.nodoc From cb736630fc10d57291b5c30b355bc87e6d2a8598 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 10 Jan 2025 15:34:37 -0800 Subject: [PATCH 02/15] Check in a test locking in that the current implementation works Test comes from issue #15824 and checks zippering two sets of the same length (but different contents). Sorts the output so that it is consistent instead of potentially racy. ---- Signed-off-by: Lydia Duncan --- test/library/standard/Set/zippering/sameLength.chpl | 12 ++++++++++++ test/library/standard/Set/zippering/sameLength.good | 13 +++++++++++++ .../standard/Set/zippering/sameLength.prediff | 3 +++ 3 files changed, 28 insertions(+) create mode 100644 test/library/standard/Set/zippering/sameLength.chpl create mode 100644 test/library/standard/Set/zippering/sameLength.good create mode 100755 test/library/standard/Set/zippering/sameLength.prediff diff --git a/test/library/standard/Set/zippering/sameLength.chpl b/test/library/standard/Set/zippering/sameLength.chpl new file mode 100644 index 000000000000..6327c44e12af --- /dev/null +++ b/test/library/standard/Set/zippering/sameLength.chpl @@ -0,0 +1,12 @@ +use Set; + +var s1, s2: set(int); + +for i in 1..10 { s1.add(i); s2.add(i+10); } + +writeln("s1 = ", s1); +writeln("s2 = ", s2); + +forall (i,j) in zip(s1, s2) do writeln(i, " ", j); + +writeln("all done"); diff --git a/test/library/standard/Set/zippering/sameLength.good b/test/library/standard/Set/zippering/sameLength.good new file mode 100644 index 000000000000..93975a4b5297 --- /dev/null +++ b/test/library/standard/Set/zippering/sameLength.good @@ -0,0 +1,13 @@ +all done +s1 = {5, 7, 8, 1, 4, 6, 2, 10, 9, 3} +s2 = {16, 20, 12, 17, 19, 13, 15, 11, 18, 14} +1 17 +2 15 +3 14 +4 19 +5 16 +6 13 +7 20 +8 12 +9 18 +10 11 diff --git a/test/library/standard/Set/zippering/sameLength.prediff b/test/library/standard/Set/zippering/sameLength.prediff new file mode 100755 index 000000000000..45315b15b8e9 --- /dev/null +++ b/test/library/standard/Set/zippering/sameLength.prediff @@ -0,0 +1,3 @@ +#!/bin/sh +sort -n $1.exec.out.tmp > $1.prediff.tmp +mv $1.prediff.tmp $1.exec.out.tmp From a62e2ca99574c029688c482e2a395c3454dd8fdd Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Mon, 13 Jan 2025 10:25:18 -0800 Subject: [PATCH 03/15] Make the leader actually parallel (oops) ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 81ed33dce3f9..60cc8a8d1186 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -691,7 +691,7 @@ module ChapelHashtable { var chunkEnds = _determineEvenChunks(numChunks, this.tableNumFullSlots); - for i in chunkEnds.domain { + forall i in chunkEnds.domain { if (i == 0) { yield (0..chunkEnds[i], 0, numChunks); } else { From 529c1c1a198015361b21a7a8b8260e7c56f13653 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Mon, 13 Jan 2025 11:05:40 -0800 Subject: [PATCH 04/15] Make the chunk yielded reflect the number of elements we want each round This is a first step towards being able to zip sets with other types instead of just other sets ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 60cc8a8d1186..4eaa69a5fa78 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -688,17 +688,18 @@ module ChapelHashtable { var numChunks = _computeNumChunks(this.tableNumFullSlots); //writeln("number of tasks = ", numChunks); //writeln("number of full slots = ", this.tableNumFullSlots); + var numFullPerTask = this.tableNumFullSlots / numChunks; + //writeln("min number of full slots per task = ", numFullPerTask); + var rem = this.tableNumFullSlots % numChunks; + //writeln("remainder = ", rem); - var chunkEnds = _determineEvenChunks(numChunks, this.tableNumFullSlots); - - forall i in chunkEnds.domain { - if (i == 0) { - yield (0..chunkEnds[i], 0, numChunks); + forall i in 0..#numChunks { + if (i < rem) { + yield (((numFullPerTask*i)+i)..<((numFullPerTask*(i+1))+(i+1)), + i, numChunks); } else { - yield ((chunkEnds[i-1]+1)..chunkEnds[i], i, numChunks); - if (i == chunkEnds.domain.high) { - yield ((chunkEnds[i]+1)..(this.tableSize-1), i+1, numChunks); - } + yield (((numFullPerTask*i)+rem)..<((numFullPerTask*(i+1))+rem), + i, numChunks); } } } From 09a4bca1d035ebd4310888006d0a2966fbdbc9da Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Mon, 13 Jan 2025 14:28:17 -0800 Subject: [PATCH 05/15] Get the set/hashtable iterator to work with arrays as well This meant always computing the number of chunks and which chunk we were ourselves, since the array iterator expects the other components of the tuple to be ranges as well (so we can't use followThis to send that information) and the argument list for the follower is supposed to match the argument list of the leader aside from followThis. This is unfortunate, but given the sizes I checked, seemed to be okay. ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 49 +++++++++++++-------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 4eaa69a5fa78..70474c8de183 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -695,15 +695,33 @@ module ChapelHashtable { forall i in 0..#numChunks { if (i < rem) { - yield (((numFullPerTask*i)+i)..<((numFullPerTask*(i+1))+(i+1)), - i, numChunks); + yield (((numFullPerTask*i)+i)..<((numFullPerTask*(i+1))+(i+1)),); } else { - yield (((numFullPerTask*i)+rem)..<((numFullPerTask*(i+1))+rem), - i, numChunks); + yield (((numFullPerTask*i)+rem)..<((numFullPerTask*(i+1))+rem),); } } } + proc _guessSlots(followThis) { + var iterSpace: range; + var intendedSpace = followThis; + var chunkSize = intendedSpace.size; + var numChunksGuess = this.tableNumFullSlots / chunkSize; + var intendedChunkGuess = intendedSpace.low / chunkSize; + + // Determine corresponding chunk to use. + var chunkEnds = _determineEvenChunks(numChunksGuess, + this.tableNumFullSlots); + if (intendedChunkGuess == 0) { + iterSpace = 0..chunkEnds[intendedChunkGuess]; + } else if (intendedChunkGuess == chunkEnds.size) { + iterSpace = (chunkEnds[intendedChunkGuess-1]+1)..(this.tableSize - 1); + } else { + iterSpace = (chunkEnds[intendedChunkGuess-1]+1)..chunkEnds[intendedChunkGuess]; + } + return iterSpace; + } + iter _evenSlots(followThis, param tag) const ref where tag == iterKind.follower { //writeln("in follower: ", followThis); @@ -712,28 +730,9 @@ module ChapelHashtable { var iterSpace: range; if (isTuple(followThis)) { - if (followThis.size == 3) { - // Using _evenSlots iterator gives us a specific set up to iterate - // over the full chunks in an even manner. - var intendedSpace = followThis(0); - var intendedChunk = followThis(1); - var numChunks = followThis(2); - - // Determine corresponding chunk to use. - var chunkEnds = _determineEvenChunks(numChunks, - this.tableNumFullSlots); - if (intendedChunk == 0) { - iterSpace = 0..chunkEnds[intendedChunk]; - } else if (intendedChunk == chunkEnds.size) { - iterSpace = (chunkEnds[intendedChunk-1]+1)..(this.tableSize - 1); - } else { - iterSpace = (chunkEnds[intendedChunk-1]+1)..chunkEnds[intendedChunk]; - } - } else { - iterSpace = followThis(0); - } + iterSpace = _guessSlots(followThis(0)); } else { - iterSpace = followThis; + iterSpace = _guessSlots(followThis); } foreach slot in iterSpace { From 75f5c49bc7a7fbf1bc4cbb9a7fcdee9926c70506 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Mon, 13 Jan 2025 14:34:28 -0800 Subject: [PATCH 06/15] Add three tests locking in the behavior when zippering with arrays Covers: - converting the set to an array and then zippering - zippering with the array leading - zippering with an array when the set leads With the prior commit, all three work (though toArray already worked prior to this effort). ---- Signed-off-by: Lydia Duncan --- .../standard/Set/zippering/toArray.chpl | 17 +++++++++++++++++ .../standard/Set/zippering/toArray.good | 3 +++ .../standard/Set/zippering/withArray1.chpl | 18 ++++++++++++++++++ .../standard/Set/zippering/withArray1.good | 2 ++ .../standard/Set/zippering/withArray2.chpl | 18 ++++++++++++++++++ .../standard/Set/zippering/withArray2.good | 2 ++ 6 files changed, 60 insertions(+) create mode 100644 test/library/standard/Set/zippering/toArray.chpl create mode 100644 test/library/standard/Set/zippering/toArray.good create mode 100644 test/library/standard/Set/zippering/withArray1.chpl create mode 100644 test/library/standard/Set/zippering/withArray1.good create mode 100644 test/library/standard/Set/zippering/withArray2.chpl create mode 100644 test/library/standard/Set/zippering/withArray2.good diff --git a/test/library/standard/Set/zippering/toArray.chpl b/test/library/standard/Set/zippering/toArray.chpl new file mode 100644 index 000000000000..2c741a80a5ef --- /dev/null +++ b/test/library/standard/Set/zippering/toArray.chpl @@ -0,0 +1,17 @@ +use Set; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A : [0..4] int; + +writeln("A = ", A); +writeln("LocalSet = ", LocalSet); +forall (a,b) in zip (A,LocalSet.toArray()) { + a=b; +} +writeln("A = ", A); diff --git a/test/library/standard/Set/zippering/toArray.good b/test/library/standard/Set/zippering/toArray.good new file mode 100644 index 000000000000..9015a912ae63 --- /dev/null +++ b/test/library/standard/Set/zippering/toArray.good @@ -0,0 +1,3 @@ +A = 0 0 0 0 0 +LocalSet = {5, 1, 4, 2, 3} +A = 5 1 4 2 3 diff --git a/test/library/standard/Set/zippering/withArray1.chpl b/test/library/standard/Set/zippering/withArray1.chpl new file mode 100644 index 000000000000..0c9270424f8f --- /dev/null +++ b/test/library/standard/Set/zippering/withArray1.chpl @@ -0,0 +1,18 @@ +use Set; + +config const verbose = false; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A : [0..4] int; +writeln(A.size, " ", LocalSet.size); +forall (a,b) in zip(A,LocalSet) { + a=b; + if verbose then writeln(b); +} +writeln(A); diff --git a/test/library/standard/Set/zippering/withArray1.good b/test/library/standard/Set/zippering/withArray1.good new file mode 100644 index 000000000000..025b58787d09 --- /dev/null +++ b/test/library/standard/Set/zippering/withArray1.good @@ -0,0 +1,2 @@ +5 5 +5 1 4 2 3 diff --git a/test/library/standard/Set/zippering/withArray2.chpl b/test/library/standard/Set/zippering/withArray2.chpl new file mode 100644 index 000000000000..e36d8beca542 --- /dev/null +++ b/test/library/standard/Set/zippering/withArray2.chpl @@ -0,0 +1,18 @@ +use Set; + +config const verbose = false; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A : [0..4] int; +writeln(A.size, " ", LocalSet.size); +forall (b, a) in zip(LocalSet, A) { + a=b; + if verbose then writeln(b); +} +writeln(A); diff --git a/test/library/standard/Set/zippering/withArray2.good b/test/library/standard/Set/zippering/withArray2.good new file mode 100644 index 000000000000..025b58787d09 --- /dev/null +++ b/test/library/standard/Set/zippering/withArray2.good @@ -0,0 +1,2 @@ +5 5 +5 1 4 2 3 From 7ee4eeaaea6fbf89f14f172c18ef58f1c51765f9 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Mon, 13 Jan 2025 15:01:06 -0800 Subject: [PATCH 07/15] Add a version of setZipDifferentSize that reverses the order of the sets The original is only failing on one of the two machines I checked, so I'm curious if this also fails there or if it behaves as expected (it passes on the machine where the original still works) ---- Signed-off-by: Lydia Duncan --- .../standard/Set/these/setZipDifferentSize2.chpl | 16 ++++++++++++++++ .../standard/Set/these/setZipDifferentSize2.good | 1 + .../Set/these/setZipDifferentSize2.skipif | 2 ++ 3 files changed, 19 insertions(+) create mode 100644 test/library/standard/Set/these/setZipDifferentSize2.chpl create mode 100644 test/library/standard/Set/these/setZipDifferentSize2.good create mode 100644 test/library/standard/Set/these/setZipDifferentSize2.skipif diff --git a/test/library/standard/Set/these/setZipDifferentSize2.chpl b/test/library/standard/Set/these/setZipDifferentSize2.chpl new file mode 100644 index 000000000000..80d0a286f44d --- /dev/null +++ b/test/library/standard/Set/these/setZipDifferentSize2.chpl @@ -0,0 +1,16 @@ +use Set; + +proc test() { + var s1: set(int); + for i in 1..8 do s1.add(i); + + var s2 = s1; + s2.add(9); + + forall (x, y) in zip(s2, s1) do + var tmp = x + y; + + return; +} +test(); + diff --git a/test/library/standard/Set/these/setZipDifferentSize2.good b/test/library/standard/Set/these/setZipDifferentSize2.good new file mode 100644 index 000000000000..a70f25b45340 --- /dev/null +++ b/test/library/standard/Set/these/setZipDifferentSize2.good @@ -0,0 +1 @@ +setZipDifferentSize2.chpl:10: error: zippered iterations have non-equal lengths diff --git a/test/library/standard/Set/these/setZipDifferentSize2.skipif b/test/library/standard/Set/these/setZipDifferentSize2.skipif new file mode 100644 index 000000000000..63ec7dcceb0b --- /dev/null +++ b/test/library/standard/Set/these/setZipDifferentSize2.skipif @@ -0,0 +1,2 @@ +# Follower checks are skipped when compiling with --fast +COMPOPTS <= --fast From 602bc5557243e1164ea5b352e0c9f765820afd96 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Thu, 16 Jan 2025 14:17:48 -0800 Subject: [PATCH 08/15] Catch case where leader has more elements than follower, consistently ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 70474c8de183..212ef5475482 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -709,6 +709,11 @@ module ChapelHashtable { var numChunksGuess = this.tableNumFullSlots / chunkSize; var intendedChunkGuess = intendedSpace.low / chunkSize; + if (intendedChunkGuess >= numChunksGuess) { + __primitive("chpl_error", + "zippered iterations have non-equal lengths".c_str()); + } + // Determine corresponding chunk to use. var chunkEnds = _determineEvenChunks(numChunksGuess, this.tableNumFullSlots); From a11a0ea72276a21dd415862f98230430a0cad7bf Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 17 Jan 2025 08:15:21 -0800 Subject: [PATCH 09/15] Try sending the number of slots in explicitly I'm not sure if this will actually allow us to check for unequal lengths, but try it and see ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 15 ++++++++++----- modules/standard/Set.chpl | 5 ++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 212ef5475482..1d22dbd7d2d2 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -684,13 +684,13 @@ module ChapelHashtable { return chunkEnds; } - iter _evenSlots(param tag) where tag == iterKind.leader { - var numChunks = _computeNumChunks(this.tableNumFullSlots); + iter _evenSlots(size: int, param tag) where tag == iterKind.leader { + var numChunks = _computeNumChunks(size); //writeln("number of tasks = ", numChunks); //writeln("number of full slots = ", this.tableNumFullSlots); - var numFullPerTask = this.tableNumFullSlots / numChunks; + var numFullPerTask = size / numChunks; //writeln("min number of full slots per task = ", numFullPerTask); - var rem = this.tableNumFullSlots % numChunks; + var rem = size % numChunks; //writeln("remainder = ", rem); forall i in 0..#numChunks { @@ -727,11 +727,16 @@ module ChapelHashtable { return iterSpace; } - iter _evenSlots(followThis, param tag) const ref + iter _evenSlots(size: int, followThis, param tag) const ref where tag == iterKind.follower { //writeln("in follower: ", followThis); use Types; + if (size != this.tableNumFullSlots) { + __primitive("chpl_error", + "zippered iterations have non-equal lengths".c_str()); + } + var iterSpace: range; if (isTuple(followThis)) { diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index 566921df1726..2409d9c12817 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -563,7 +563,7 @@ module Set { @chpldoc.nodoc iter const these(param tag) where tag == iterKind.leader { - for followThis in _htb._evenSlots(tag) { + for followThis in _htb._evenSlots(_htb.tableNumFullSlots, tag) { //writeln(followThis); yield followThis; } @@ -578,8 +578,7 @@ module Set { @chpldoc.nodoc iter const these(param tag, followThis) const ref where tag == iterKind.follower { - //writeln(this); - foreach val in _htb._evenSlots(followThis, tag) { + foreach val in _htb._evenSlots(_htb.tableNumFullSlots, followThis, tag) { yield val; } /* From 4ca63231574d9e3e57b9214dce90e733e89c60c9 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Wed, 22 Jan 2025 09:51:54 -0800 Subject: [PATCH 10/15] Try adding an alternate iterator to Set that takes a size argument This argument indicates the size of the leader in the iteration, so that we can validate if the iteration space is uneven. It has a default value so that the non-zippered case is not as annoying to write Update various tests to use this iterator instead ---- Signed-off-by: Lydia Duncan --- modules/standard/Set.chpl | 48 +++++++++++++++++++ .../Set/these/setZipDifferentSize.chpl | 2 +- .../Set/these/setZipDifferentSize2.chpl | 2 +- .../standard/Set/zippering/sameLength.chpl | 3 +- .../standard/Set/zippering/withArray1.chpl | 2 +- .../standard/Set/zippering/withArray2.chpl | 2 +- 6 files changed, 54 insertions(+), 5 deletions(-) diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index 2409d9c12817..df5ac1ab6e8f 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -55,6 +55,7 @@ module Set { private use Reflection; private use ChapelHashtable; private use HaltWrappers; + private use CTypes; @chpldoc.nodoc private param _sanityChecks = true; @@ -587,6 +588,53 @@ module Set { */ } + iter const contents(size: int = _htb.tableNumFullSlots) const ref { + if (size != _htb.tableNumFullSlots) { + __primitive("chpl_error", + "didn't specify the full contents of the set".c_str()); + } + + foreach idx in 0..#_htb.tableSize { + if _htb.isSlotFull(idx) then yield _htb.table[idx].key; + } + } + + @chpldoc.nodoc + iter const contents(size: int = _htb.tableNumFullSlots, param tag) const ref + where tag == iterKind.standalone { + if (size != _htb.tableNumFullSlots) { + __primitive("chpl_error", + "didn't specify the full contents of the set".c_str()); + } + + var space = 0..#_htb.tableSize; + foreach idx in space.these(tag) do + if _htb.isSlotFull(idx) then yield _htb.table[idx].key; + } + + @chpldoc.nodoc + iter const contents(size: int = _htb.tableNumFullSlots, param tag) + where tag == iterKind.leader { + if (size != _htb.tableNumFullSlots) { + __primitive("chpl_error", + "didn't specify the full contents of the set".c_str()); + } + + for followThis in _htb._evenSlots(size, tag) { + //writeln(followThis); + yield followThis; + } + } + + @chpldoc.nodoc + iter const contents(size: int = _htb.tableNumFullSlots, param tag, + followThis) const ref + where tag == iterKind.follower { + foreach val in _htb._evenSlots(size, followThis, tag) { + yield val; + } + } + @chpldoc.nodoc proc const _defaultWriteHelper(ch: fileWriter) throws { on this { diff --git a/test/library/standard/Set/these/setZipDifferentSize.chpl b/test/library/standard/Set/these/setZipDifferentSize.chpl index 289a524c2aba..dffcecb6e1ab 100644 --- a/test/library/standard/Set/these/setZipDifferentSize.chpl +++ b/test/library/standard/Set/these/setZipDifferentSize.chpl @@ -7,7 +7,7 @@ proc test() { var s2 = s1; s2.add(9); - forall (x, y) in zip(s1, s2) do + forall (x, y) in zip(s1.contents(s1.size), s2.contents(s1.size)) do var tmp = x + y; return; diff --git a/test/library/standard/Set/these/setZipDifferentSize2.chpl b/test/library/standard/Set/these/setZipDifferentSize2.chpl index 80d0a286f44d..10d843f01176 100644 --- a/test/library/standard/Set/these/setZipDifferentSize2.chpl +++ b/test/library/standard/Set/these/setZipDifferentSize2.chpl @@ -7,7 +7,7 @@ proc test() { var s2 = s1; s2.add(9); - forall (x, y) in zip(s2, s1) do + forall (x, y) in zip(s2.contents(s2.size), s1.contents(s2.size)) do var tmp = x + y; return; diff --git a/test/library/standard/Set/zippering/sameLength.chpl b/test/library/standard/Set/zippering/sameLength.chpl index 6327c44e12af..5858be8e2ac4 100644 --- a/test/library/standard/Set/zippering/sameLength.chpl +++ b/test/library/standard/Set/zippering/sameLength.chpl @@ -7,6 +7,7 @@ for i in 1..10 { s1.add(i); s2.add(i+10); } writeln("s1 = ", s1); writeln("s2 = ", s2); -forall (i,j) in zip(s1, s2) do writeln(i, " ", j); +forall (i,j) in zip(s1.contents(s1.size), s2.contents(s1.size)) do + writeln(i, " ", j); writeln("all done"); diff --git a/test/library/standard/Set/zippering/withArray1.chpl b/test/library/standard/Set/zippering/withArray1.chpl index 0c9270424f8f..a00cc188a8e1 100644 --- a/test/library/standard/Set/zippering/withArray1.chpl +++ b/test/library/standard/Set/zippering/withArray1.chpl @@ -11,7 +11,7 @@ LocalSet.add(5); var A : [0..4] int; writeln(A.size, " ", LocalSet.size); -forall (a,b) in zip(A,LocalSet) { +forall (a,b) in zip(A,LocalSet.contents(A.size)) { a=b; if verbose then writeln(b); } diff --git a/test/library/standard/Set/zippering/withArray2.chpl b/test/library/standard/Set/zippering/withArray2.chpl index e36d8beca542..83866c7958a1 100644 --- a/test/library/standard/Set/zippering/withArray2.chpl +++ b/test/library/standard/Set/zippering/withArray2.chpl @@ -11,7 +11,7 @@ LocalSet.add(5); var A : [0..4] int; writeln(A.size, " ", LocalSet.size); -forall (b, a) in zip(LocalSet, A) { +forall (b, a) in zip(LocalSet.contents(LocalSet.size), A) { a=b; if verbose then writeln(b); } From dab66050d0766f044260bbf2c98563c45444c25b Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Wed, 22 Jan 2025 14:00:49 -0800 Subject: [PATCH 11/15] Cleanups - document new iterator - remove commented out writelns - restore old behavior to Set's these iterators, since we're using a different iterator - comment an if statement in _determineEvenChunks - close a parenthetical in an older comment in ChapelHashtable ---- Signed-off-by: Lydia Duncan --- modules/internal/ChapelHashtable.chpl | 17 ++++------------ modules/standard/Set.chpl | 29 ++++++++++++++++----------- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 1d22dbd7d2d2..9092b5b194d1 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -144,7 +144,7 @@ module ChapelHashtable { const minSizePerTask = dataParMinGranularity; // We are simply slicing up the table here. Trying to do something - // more intelligent (like evenly dividing up the full slots, led + // more intelligent (like evenly dividing up the full slots), led // to poor speed ups. if debugAssocDataPar { @@ -647,9 +647,7 @@ module ChapelHashtable { proc _determineEvenChunks(numChunks: int, numFull: int): [] int { var numFullPerTask = numFull / numChunks; - //writeln("min number of full slots per task = ", numFullPerTask); var rem = numFull % numChunks; - //writeln("remainder = ", rem); var chunkEnds: [0..#(numChunks-1)] int; var chunkIdx = 0; @@ -664,34 +662,28 @@ module ChapelHashtable { (chunkIdx >= chunkEnds.size) { break; } + + // Allows us to evenly distribute when the chunks don't divide the + // total number of elements evenly. if chunkIdx < rem { if (curNumFull == (numFullPerTask * (chunkIdx+1)) + (chunkIdx+1)) { - //writeln("index ", i, " is getting saved in chunkEnds[", chunkIdx, - // "]"); chunkEnds[chunkIdx] = i; chunkIdx += 1; } } else { if (curNumFull == (numFullPerTask * (chunkIdx+1)) + rem) { - //writeln("index ", i, " is getting saved in chunkEnds[", chunkIdx, - // "]"); chunkEnds[chunkIdx] = i; chunkIdx += 1; } } } - //writeln("chunkEnds = ", chunkEnds); return chunkEnds; } iter _evenSlots(size: int, param tag) where tag == iterKind.leader { var numChunks = _computeNumChunks(size); - //writeln("number of tasks = ", numChunks); - //writeln("number of full slots = ", this.tableNumFullSlots); var numFullPerTask = size / numChunks; - //writeln("min number of full slots per task = ", numFullPerTask); var rem = size % numChunks; - //writeln("remainder = ", rem); forall i in 0..#numChunks { if (i < rem) { @@ -729,7 +721,6 @@ module ChapelHashtable { iter _evenSlots(size: int, followThis, param tag) const ref where tag == iterKind.follower { - //writeln("in follower: ", followThis); use Types; if (size != this.tableNumFullSlots) { diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index df5ac1ab6e8f..855d8fdf7648 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -564,30 +564,36 @@ module Set { @chpldoc.nodoc iter const these(param tag) where tag == iterKind.leader { - for followThis in _htb._evenSlots(_htb.tableNumFullSlots, tag) { - //writeln(followThis); - yield followThis; - } - /* var space = 0..#_htb.tableSize; for followThis in space.these(tag) { yield followThis; } - */ } @chpldoc.nodoc iter const these(param tag, followThis) const ref where tag == iterKind.follower { - foreach val in _htb._evenSlots(_htb.tableNumFullSlots, followThis, tag) { - yield val; - } - /* foreach idx in followThis(0) do if _htb.isSlotFull(idx) then yield _htb.table[idx].key; - */ } + /* + Iterate over the elements of this set. Yields constant references + that cannot be modified. + + .. warning:: + + Modifying this set while iterating over it may invalidate the + references returned by an iterator and is considered undefined + behavior. + + :yields: A constant reference to an element in this set. + + :arg size: the number of elements in the leader, when participating in + zippered parallel iteration. Defaults to the number of + elements in the set. For standalone and serial iteration, this + must match the number of elements in the set. + */ iter const contents(size: int = _htb.tableNumFullSlots) const ref { if (size != _htb.tableNumFullSlots) { __primitive("chpl_error", @@ -621,7 +627,6 @@ module Set { } for followThis in _htb._evenSlots(size, tag) { - //writeln(followThis); yield followThis; } } From 375e1aa8cae033734c2f37c7a4b4c184306d4b78 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Wed, 22 Jan 2025 14:17:28 -0800 Subject: [PATCH 12/15] Use size as the default instead of the internal information It'll present a cleaner interface to users ---- Signed-off-by: Lydia Duncan --- modules/standard/Set.chpl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index 855d8fdf7648..ed5ae0b526f6 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -594,7 +594,7 @@ module Set { elements in the set. For standalone and serial iteration, this must match the number of elements in the set. */ - iter const contents(size: int = _htb.tableNumFullSlots) const ref { + iter const contents(size: int = this.size) const ref { if (size != _htb.tableNumFullSlots) { __primitive("chpl_error", "didn't specify the full contents of the set".c_str()); @@ -606,7 +606,7 @@ module Set { } @chpldoc.nodoc - iter const contents(size: int = _htb.tableNumFullSlots, param tag) const ref + iter const contents(size: int = this.size, param tag) const ref where tag == iterKind.standalone { if (size != _htb.tableNumFullSlots) { __primitive("chpl_error", @@ -619,7 +619,7 @@ module Set { } @chpldoc.nodoc - iter const contents(size: int = _htb.tableNumFullSlots, param tag) + iter const contents(size: int = this.size, param tag) where tag == iterKind.leader { if (size != _htb.tableNumFullSlots) { __primitive("chpl_error", @@ -632,7 +632,7 @@ module Set { } @chpldoc.nodoc - iter const contents(size: int = _htb.tableNumFullSlots, param tag, + iter const contents(size: int = this.size, param tag, followThis) const ref where tag == iterKind.follower { foreach val in _htb._evenSlots(size, followThis, tag) { From 336dd2c920bef01fca142936d32551ae8d928190 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Wed, 22 Jan 2025 15:27:47 -0800 Subject: [PATCH 13/15] Add some more tests Covers: - Zippering with a shorter array as leader - Zippering with a shorter array as follower - Serial version of the new iterator - Standalone version of the new iterator ---- Signed-off-by: Lydia Duncan --- .../standard/Set/zippering/arrayShorter.chpl | 20 +++++++++++++++++++ .../standard/Set/zippering/arrayShorter.good | 2 ++ .../standard/Set/zippering/arrayShorter2.chpl | 20 +++++++++++++++++++ .../standard/Set/zippering/arrayShorter2.good | 2 ++ .../standard/Set/zippering/serialVersion.chpl | 19 ++++++++++++++++++ .../standard/Set/zippering/serialVersion.good | 10 ++++++++++ .../Set/zippering/standaloneVersion.chpl | 20 +++++++++++++++++++ .../Set/zippering/standaloneVersion.good | 10 ++++++++++ .../Set/zippering/standaloneVersion.prediff | 3 +++ 9 files changed, 106 insertions(+) create mode 100644 test/library/standard/Set/zippering/arrayShorter.chpl create mode 100644 test/library/standard/Set/zippering/arrayShorter.good create mode 100644 test/library/standard/Set/zippering/arrayShorter2.chpl create mode 100644 test/library/standard/Set/zippering/arrayShorter2.good create mode 100644 test/library/standard/Set/zippering/serialVersion.chpl create mode 100644 test/library/standard/Set/zippering/serialVersion.good create mode 100644 test/library/standard/Set/zippering/standaloneVersion.chpl create mode 100644 test/library/standard/Set/zippering/standaloneVersion.good create mode 100755 test/library/standard/Set/zippering/standaloneVersion.prediff diff --git a/test/library/standard/Set/zippering/arrayShorter.chpl b/test/library/standard/Set/zippering/arrayShorter.chpl new file mode 100644 index 000000000000..8d8500d2c63a --- /dev/null +++ b/test/library/standard/Set/zippering/arrayShorter.chpl @@ -0,0 +1,20 @@ +use Set; + +config const verbose = false; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A: [0..3] int; +writeln(A.size, " ", LocalSet.size); + +// Expect this to fail to zip, the set is longer than the array +forall (a,b) in zip(A,LocalSet.contents(A.size)) { + a=b; + if verbose then writeln(b); +} +writeln(A); diff --git a/test/library/standard/Set/zippering/arrayShorter.good b/test/library/standard/Set/zippering/arrayShorter.good new file mode 100644 index 000000000000..3fc6e8038f67 --- /dev/null +++ b/test/library/standard/Set/zippering/arrayShorter.good @@ -0,0 +1,2 @@ +4 5 +arrayShorter.chpl:16: error: zippered iterations have non-equal lengths diff --git a/test/library/standard/Set/zippering/arrayShorter2.chpl b/test/library/standard/Set/zippering/arrayShorter2.chpl new file mode 100644 index 000000000000..82d3c8c2c7eb --- /dev/null +++ b/test/library/standard/Set/zippering/arrayShorter2.chpl @@ -0,0 +1,20 @@ +use Set; + +config const verbose = false; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A: [0..3] int; +writeln(A.size, " ", LocalSet.size); + +// Expect this to fail to zip, the set is longer than the array +forall (b, a) in zip(LocalSet.contents(LocalSet.size), A) { + a=b; + if verbose then writeln(b); +} +writeln(A); diff --git a/test/library/standard/Set/zippering/arrayShorter2.good b/test/library/standard/Set/zippering/arrayShorter2.good new file mode 100644 index 000000000000..48d873b00071 --- /dev/null +++ b/test/library/standard/Set/zippering/arrayShorter2.good @@ -0,0 +1,2 @@ +4 5 +arrayShorter2.chpl:16: error: halt reached - size mismatch in zippered iteration (dimension 0) diff --git a/test/library/standard/Set/zippering/serialVersion.chpl b/test/library/standard/Set/zippering/serialVersion.chpl new file mode 100644 index 000000000000..68029504b8f1 --- /dev/null +++ b/test/library/standard/Set/zippering/serialVersion.chpl @@ -0,0 +1,19 @@ +use Set; + +var s = new set(string); + +s.add("my"); +s.add("great"); +s.add("aunt"); +s.add("sally"); +s.add("told"); +s.add("me"); +s.add("to"); +s.add("sleep"); +s.add("in"); +s.add("tomorrow"); + +// Check the behavior of the contents iterator when used in a serial context +for val in s.contents() { + writeln(val); +} diff --git a/test/library/standard/Set/zippering/serialVersion.good b/test/library/standard/Set/zippering/serialVersion.good new file mode 100644 index 000000000000..2c91268a704f --- /dev/null +++ b/test/library/standard/Set/zippering/serialVersion.good @@ -0,0 +1,10 @@ +great +in +tomorrow +sleep +aunt +me +sally +my +told +to diff --git a/test/library/standard/Set/zippering/standaloneVersion.chpl b/test/library/standard/Set/zippering/standaloneVersion.chpl new file mode 100644 index 000000000000..7ca7fc9a22f9 --- /dev/null +++ b/test/library/standard/Set/zippering/standaloneVersion.chpl @@ -0,0 +1,20 @@ +use Set; + +var s = new set(string); + +s.add("my"); +s.add("great"); +s.add("aunt"); +s.add("sally"); +s.add("told"); +s.add("me"); +s.add("to"); +s.add("sleep"); +s.add("in"); +s.add("tomorrow"); + +// Check the behavior of the contents iterator when used as a standalone +// iterator +forall val in s.contents() { + writeln(val); +} diff --git a/test/library/standard/Set/zippering/standaloneVersion.good b/test/library/standard/Set/zippering/standaloneVersion.good new file mode 100644 index 000000000000..6791a2bf0ab5 --- /dev/null +++ b/test/library/standard/Set/zippering/standaloneVersion.good @@ -0,0 +1,10 @@ +aunt +great +in +me +my +sally +sleep +to +told +tomorrow diff --git a/test/library/standard/Set/zippering/standaloneVersion.prediff b/test/library/standard/Set/zippering/standaloneVersion.prediff new file mode 100755 index 000000000000..45315b15b8e9 --- /dev/null +++ b/test/library/standard/Set/zippering/standaloneVersion.prediff @@ -0,0 +1,3 @@ +#!/bin/sh +sort -n $1.exec.out.tmp > $1.prediff.tmp +mv $1.prediff.tmp $1.exec.out.tmp From 2f33436e1e6e1522ec12f9d12ba2db8fe6390886 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Thu, 23 Jan 2025 13:37:10 -0800 Subject: [PATCH 14/15] Add an unstable warning to one of the iterator overloads and a test of it Would have added a warning to all the overloads, except for the bug reported in all the right contexts, whether the bug or the unstable warning gets resolved first. ---- Signed-off-by: Lydia Duncan --- modules/standard/Set.chpl | 1 + test/unstable/Set/contentsIterator.chpl | 69 +++++++++++++++++++++++++ test/unstable/Set/contentsIterator.good | 22 ++++++++ 3 files changed, 92 insertions(+) create mode 100644 test/unstable/Set/contentsIterator.chpl create mode 100644 test/unstable/Set/contentsIterator.good diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index ed5ae0b526f6..67868d44e84a 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -594,6 +594,7 @@ module Set { elements in the set. For standalone and serial iteration, this must match the number of elements in the set. */ + @unstable("The contents iterator is unstable because it is new") iter const contents(size: int = this.size) const ref { if (size != _htb.tableNumFullSlots) { __primitive("chpl_error", diff --git a/test/unstable/Set/contentsIterator.chpl b/test/unstable/Set/contentsIterator.chpl new file mode 100644 index 000000000000..221cc9abc9ba --- /dev/null +++ b/test/unstable/Set/contentsIterator.chpl @@ -0,0 +1,69 @@ +// Check we warn unstable for all versions of the iterator +// Lydia note: Adding because I'm only adding the unstable attribute to one +// overload because of #26590 and if we fix that before stabilizing these +// iterators, we will need to update the other ones to have the attribute + +use Set; + +var s = new set(string); + +s.add("my"); +s.add("great"); +s.add("aunt"); +s.add("sally"); +s.add("told"); +s.add("me"); +s.add("to"); +s.add("sleep"); +s.add("in"); +s.add("tomorrow"); + +// Check the behavior of the contents iterator when used in a serial context +for val in s.contents() { + writeln(val); +} + +var checkStandalone = new set(int, parSafe = true); +checkStandalone.add(1); +checkStandalone.add(2); +checkStandalone.add(3); +checkStandalone.add(4); +checkStandalone.add(5); +var sum: atomic int; + +// Check the behavior of the contents iterator when used in a standalone context +forall val in checkStandalone.contents() { + sum.add(val); +} + +writeln(sum.read()); + +var LocalSet = new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A : [0..4] int; +writeln(A.size, " ", LocalSet.size); +// Check the behavior of the contents iterator when used in a follower context +forall (a,b) in zip(A,LocalSet.contents(A.size)) { + a=b; +} +writeln(A); + +var LocalSet2= new set(int,parSafe = true); +LocalSet2.add(1); +LocalSet2.add(2); +LocalSet2.add(3); +LocalSet2.add(5); +LocalSet2.add(7); + +var A2 : [0..4] int; +writeln(A2.size, " ", LocalSet2.size); +// Check the behavior of the contents iterator when used in a leader context +forall (b, a) in zip(LocalSet2.contents(LocalSet2.size), A2) { + a=b; +} +writeln(A2); diff --git a/test/unstable/Set/contentsIterator.good b/test/unstable/Set/contentsIterator.good new file mode 100644 index 000000000000..3c1af8d025db --- /dev/null +++ b/test/unstable/Set/contentsIterator.good @@ -0,0 +1,22 @@ +contentsIterator.chpl:22: warning: The contents iterator is unstable because it is new +contentsIterator.chpl:26: warning: 'set.parSafe' is unstable and is expected to be replaced by a separate set type in the future +contentsIterator.chpl:35: warning: The contents iterator is unstable because it is new +contentsIterator.chpl:41: warning: 'set.parSafe' is unstable and is expected to be replaced by a separate set type in the future +contentsIterator.chpl:51: warning: The contents iterator is unstable because it is new +contentsIterator.chpl:56: warning: 'set.parSafe' is unstable and is expected to be replaced by a separate set type in the future +contentsIterator.chpl:66: warning: The contents iterator is unstable because it is new +great +in +tomorrow +sleep +aunt +me +sally +my +told +to +15 +5 5 +5 1 4 2 3 +5 5 +5 7 1 2 3 From 1cdbbf79d5e282339849050af506273506078b04 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Thu, 23 Jan 2025 15:12:48 -0800 Subject: [PATCH 15/15] Improve the error message for Set's part of the iterators Use a halt wrapper instead of `__primitive("chpl_error"`, especially because this is a new error instead of trying to match errors that the compiler can insert. While doing that, made the error messages clearer about what was wrong and what should be done instead (and hopefully head off confusion for the follower case). And add a test locking in the error message in the leader case ---- Signed-off-by: Lydia Duncan --- modules/standard/Set.chpl | 14 +++++------ .../Set/zippering/badContentsCall.chpl | 23 +++++++++++++++++++ .../Set/zippering/badContentsCall.good | 2 ++ 3 files changed, 32 insertions(+), 7 deletions(-) create mode 100644 test/library/standard/Set/zippering/badContentsCall.chpl create mode 100644 test/library/standard/Set/zippering/badContentsCall.good diff --git a/modules/standard/Set.chpl b/modules/standard/Set.chpl index 67868d44e84a..507b6a89703c 100644 --- a/modules/standard/Set.chpl +++ b/modules/standard/Set.chpl @@ -55,7 +55,6 @@ module Set { private use Reflection; private use ChapelHashtable; private use HaltWrappers; - private use CTypes; @chpldoc.nodoc private param _sanityChecks = true; @@ -597,8 +596,8 @@ module Set { @unstable("The contents iterator is unstable because it is new") iter const contents(size: int = this.size) const ref { if (size != _htb.tableNumFullSlots) { - __primitive("chpl_error", - "didn't specify the full contents of the set".c_str()); + iterHalt("serial 'contents' iterator needs to be provided the set's " + + "size as its argument"); } foreach idx in 0..#_htb.tableSize { @@ -610,8 +609,8 @@ module Set { iter const contents(size: int = this.size, param tag) const ref where tag == iterKind.standalone { if (size != _htb.tableNumFullSlots) { - __primitive("chpl_error", - "didn't specify the full contents of the set".c_str()); + iterHalt("standalone 'contents' iterator needs to be provided the " + + "set's size as its argument"); } var space = 0..#_htb.tableSize; @@ -623,8 +622,9 @@ module Set { iter const contents(size: int = this.size, param tag) where tag == iterKind.leader { if (size != _htb.tableNumFullSlots) { - __primitive("chpl_error", - "didn't specify the full contents of the set".c_str()); + iterHalt("leader 'contents' iterator needs to be provided the " + + "set's size as its argument (follower should be provided the" + + " size of the leader)"); } for followThis in _htb._evenSlots(size, tag) { diff --git a/test/library/standard/Set/zippering/badContentsCall.chpl b/test/library/standard/Set/zippering/badContentsCall.chpl new file mode 100644 index 000000000000..02d5b654b9e5 --- /dev/null +++ b/test/library/standard/Set/zippering/badContentsCall.chpl @@ -0,0 +1,23 @@ +// Test to verify that we yell when sending in a bad length + +use Set; + +config const verbose = false; + +var LocalSet= new set(int,parSafe = true); +LocalSet.add(1); +LocalSet.add(2); +LocalSet.add(3); +LocalSet.add(4); +LocalSet.add(5); + +var A: [0..3] int; +writeln(A.size, " ", LocalSet.size); + +// Expect this to fail with our error message, the leader needs to be sent its +// own size (I know, I know, I'm open to alternative implementations) +forall (b, a) in zip(LocalSet.contents(A.size), A) { + a=b; + if verbose then writeln(b); +} +writeln(A); diff --git a/test/library/standard/Set/zippering/badContentsCall.good b/test/library/standard/Set/zippering/badContentsCall.good new file mode 100644 index 000000000000..45ee29906eba --- /dev/null +++ b/test/library/standard/Set/zippering/badContentsCall.good @@ -0,0 +1,2 @@ +4 5 +badContentsCall.chpl:19: error: halt reached - leader 'contents' iterator needs to be provided the set's size as its argument (follower should be provided the size of the leader)