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

Memories sent fix #1075

Merged
merged 7 commits into from
Jul 7, 2022
Merged

Memories sent fix #1075

merged 7 commits into from
Jul 7, 2022

Conversation

nathanielnrn
Copy link
Contributor

@nathanielnrn nathanielnrn commented Jul 6, 2022

Fixes a memories_sent assignment part of #1072, part of general effort in #1022

Memories_sent is now asserted when all of the x_send_done signals are
asserted. Before, each bit corresponded to a different memory being
done.

This avoids having computations finish when any one of the memories
is done.
format!("{}_send_done", mem),
));
}
ifelse.add_seq(v::Sequential::new_nonblk_assign(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the if memories.len() == 1 stuff? Seems like fold will do the right thing when memories[1..] is empty? We should still probably have an assert!(..) saying that it should have at least one element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think you're right, made the changes

nathanielnrn and others added 3 commits July 6, 2022 16:37
We assume that at least 1 memory is present, assertion is
made at top of toplevel function
@nathanielnrn nathanielnrn enabled auto-merge (squash) July 6, 2022 21:06
@rachitnigam
Copy link
Contributor

LGTM!

@nathanielnrn nathanielnrn enabled auto-merge (squash) July 7, 2022 13:26
@nathanielnrn nathanielnrn merged commit ce7fce0 into master Jul 7, 2022
@nathanielnrn nathanielnrn deleted the memories-sent-fix branch July 7, 2022 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants