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

Fix issues in distBag.addBulk #26657

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Guillaume-Helbecque
Copy link
Contributor

@Guillaume-Helbecque Guillaume-Helbecque commented Feb 5, 2025

In the addBulk procedure of the distBag data structure, things are as follows:

inline proc ref addElements(elts): int
    {
      var size = elts.size;
      ...
      // add the elements to the tail
      for elt in elts[0..#size] do block.pushTail(elt);
      tail += size;
      ...
    }

This produces an issue when the input elts is not 0-based (e.g., array slices). For example,

const A: [0..9] int = [0,1,2,3,4,5,6,7,8,9];
var bag = new distBag(int);
bag.addBulk(A[2..9], 0);
writeln(bag);

returns [2, 3, 4, 5, 6, 7, 0, 0] instead of [2, 3, 4, 5, 6, 7, 8, 9].

A fix for this is to iterate over elts without specifying the domain, and use a counter to make sure that we do not insert more elements than we can. I also update the tests to make sure that this bug doesn't come back in the future.

Signed-off-by: Guillaume-Helbecque <helbecque.guillaume@gmail.com>
Signed-off-by: Guillaume-Helbecque <helbecque.guillaume@gmail.com>
Signed-off-by: Guillaume-Helbecque <helbecque.guillaume@gmail.com>
@Guillaume-Helbecque Guillaume-Helbecque marked this pull request as ready for review February 5, 2025 15:37
@lydia-duncan lydia-duncan self-requested a review February 5, 2025 22:34
Signed-off-by: Guillaume-Helbecque <helbecque.guillaume@gmail.com>
@Guillaume-Helbecque
Copy link
Contributor Author

While working on a test failure, I noticed that there was another tiny issue in the way we compute the number of elements to insert when the maximum capacity of a segment is reached. For some reasons, this was hidden in the previous tests. I thus provided more tests to capture this.

@Guillaume-Helbecque Guillaume-Helbecque changed the title Fix an issue in distBag.addBulk Fix issues in distBag.addBulk Feb 6, 2025
@lydia-duncan
Copy link
Member

I'm never gonna say no to more tests XD I'll try to take another look today, got a meeting coming up in five minutes

Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks! I'll merge this on Monday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants