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

Replace typedef'ed types with C++ PODs #2541

Closed
heplesser opened this issue Nov 29, 2022 · 3 comments · Fixed by #2618
Closed

Replace typedef'ed types with C++ PODs #2541

heplesser opened this issue Nov 29, 2022 · 3 comments · Fixed by #2618
Assignees
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority stale Automatic marker for inactivity, please have another look here T: Enhancement New functionality, model or documentation
Projects

Comments

@heplesser
Copy link
Contributor

This issue is a follow-up to #2215.

After a thorough discussion during the Hackathon we came to the following conclusions:

  1. Typedefs (except weight and partially delay) are only used to index into containers. Therefore, size_t is the suitable POD where currently typedefs index, thread, port, rport, ... are used.
  2. Typedefs do not provide any type safety in C++.
  3. Where we use signed types through typedefs today (int, long), we only use value -1 from the negative range, wasting value range. Use of size_t would avoid this.
  4. Named type solutions seem too complicated, as do implementing our own classes for, e.g., Thread, Delay, ....
  5. We therefore concluded that we will explore replacing all typedefed types with PODs, using unsigned PODs throughout.
  6. For marking invalid values, three approaches can be used, as suitable in the situation
    1. Return a std::pair<> combining the value and a boolean flag indicating validity.
    2. Use std::numeric_limits<POD>::max() as the invalid value.
    3. Raise exceptions instead of returning invalid values.
  7. Certain quantities have implicitly limited value ranges because they need to be transmitted in bitfields of limited size in MPI communication (
    #if TARGET_BITS_SPLIT == TARGET_BITS_SPLIT_STANDARD
    constexpr uint8_t NUM_BITS_RANK = 18U;
    constexpr uint8_t NUM_BITS_TID = 9U;
    constexpr uint8_t NUM_BITS_SYN_ID = 9U;
    #elif TARGET_BITS_SPLIT == TARGET_BITS_SPLIT_HPC
    constexpr uint8_t NUM_BITS_RANK = 20U;
    constexpr uint8_t NUM_BITS_TID = 10U;
    constexpr uint8_t NUM_BITS_SYN_ID = 6U;
    #endif
    constexpr uint8_t NUM_BITS_LCID = 27U;
    constexpr uint8_t NUM_BITS_PROCESSED_FLAG = 1U;
    constexpr uint8_t NUM_BITS_MARKER_SPIKE_DATA = 2U;
    constexpr uint8_t NUM_BITS_LAG = 14U;
    constexpr uint8_t NUM_BITS_DELAY = 21U;
    constexpr uint8_t NUM_BITS_NODE_ID = 62U;
    /*
    * Maximally allowed values for bitfields
    */
    constexpr uint64_t MAX_LCID = generate_max_value( NUM_BITS_LCID );
    constexpr int64_t MAX_RANK = generate_max_value( NUM_BITS_RANK );
    constexpr int64_t MAX_TID = generate_max_value( NUM_BITS_TID );
    constexpr uint64_t MAX_SYN_ID = generate_max_value( NUM_BITS_SYN_ID );
    constexpr uint64_t DISABLED_NODE_ID = generate_max_value( NUM_BITS_NODE_ID );
    constexpr uint64_t MAX_NODE_ID = DISABLED_NODE_ID - 1;
    ). Where these values are set (e.g., when the number of threads is changed or new synapse models are created), we need to check if new values are in the permissible range.
@heplesser heplesser added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know labels Nov 29, 2022
@heplesser heplesser added this to To do (open issues) in Kernel via automation Nov 29, 2022
@JanVogelsang
Copy link
Contributor

As an addition: port and rport are currently of type long, although I feel size_t would be more fitting. However, I would even suggest to not use size_t, but use int instead, just to make sure we never reserve 8 Bytes per port on any architecture and rport is used within the Connection class, thus contributing a lot to the overall memory consumption.

@jougs
Copy link
Contributor

jougs commented Feb 20, 2023

@poojanbabu: any news on this one?

@github-actions
Copy link

Issue automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Apr 22, 2023
Kernel automation moved this from To do (open issues) to Done (PRs and issues) Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority stale Automatic marker for inactivity, please have another look here T: Enhancement New functionality, model or documentation
Projects
Kernel
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants