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

Extdev fpgas #463

Merged
merged 109 commits into from
Sep 8, 2022
Merged

Extdev fpgas #463

merged 109 commits into from
Sep 8, 2022

Conversation

rowleya
Copy link
Member

@rowleya rowleya commented Sep 1, 2022

The main change here is to support external devices connected via the FPGAs and the addition of 2D support to vertices. The main highlights:

  • Allows a single device to be connected via multiple FPGAs, and different input and output FPGAs
  • Adds support for 2D vertices (and other dimensions technically)
  • Simplifies support of routing constraints on devices by having a simple method override rather than a constraint
  • Removes constraints that all things use anyway and all allocators allocate with anyway
  • Removes virtual chips, instead relying on algorithms being aware of how devices work

Depends on SpiNNakerManchester/SpiNNMachine#181

Copy link
Member

@Christian-B Christian-B left a comment

Choose a reason for hiding this comment

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

Lots of small comments. None are blockers but all should be address at some point (ok for version 2)

pacman/model/graphs/common/slice.py Outdated Show resolved Hide resolved
pacman/model/graphs/common/slice.py Outdated Show resolved Hide resolved
pacman/utilities/utility_calls.py Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewgait andrewgait left a comment

Choose a reason for hiding this comment

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

I'm happy with this; just made one more comment here that might be worth thinking about.


SETTING_SPLITTER_ERROR_MSG = (
"The splitter object on {} has already been set, it cannot be "
"reset. Please fix and try again. ")

def __init__(self, label=None, constraints=None,
max_atoms_per_core=sys.maxsize, splitter=None):
max_atoms_per_core=None, splitter=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether calling this variable max_atoms_per_dimension_per_core as well might be a good idea. (I'm not sure whether PyNN has named this variable already and that might mean that might not be a good idea...). I suppose because in the majority of cases it will still be a single int that it's probably not worth changing but I thought I'd put the suggestion out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did wonder the same thing but didn't for two reasons:

  1. It may be hard to find places where this is used as a keyword argument given the number of application vertices around, and how some might be called via super(...).
  2. Arguably, max_atoms_per_dimension_per_core returned elsewhere is slightly different, in that it is always a tuple, where this can be a tuple, int or None.

@rowleya rowleya merged commit 648e22b into master Sep 8, 2022
@rowleya rowleya deleted the extdev_fpgas branch September 8, 2022 11:53
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.

3 participants