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

Added length method to bank template #307

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TSonono
Copy link
Contributor

@TSonono TSonono commented Aug 6, 2024

Does not add startup time but utilizes the startup memoized method _sorted_regs() which means this method should not scale on bank length.

Feel free to close this PR if it doesn't make sense to include this in dml-builtins.

@mandolaerik
Copy link
Contributor

I'm not so concerned about startup time, so memoized would be fine (the static cost is an integer per bank, 8 to 16 bytes in total).

If we are concerned about this cost, then we can make it an optional utility template, like:

template bank_length is bank { shared independent startup memoized method length() -> (uint64) { ...}

so whoever wants it universally can say:

in each bank { is bank_length; }

@mandolaerik
Copy link
Contributor

And as usual, naming is a hard problem. "Length" is somewhat ambiguous (other interpretations: number of regs, sum of reg sizes, difference between lowest and highest offs of a register byte). One option could be to let a method return the highest-offset register object; here, static typing would help eliminating ambiguity in naming.

@mandolaerik
Copy link
Contributor

Also, we need to consider implications for custom implementations of read or write on bank level (such as "simple regs"): Would the existence of a length method mean that read/write overrides are expected to override the length method, to return the largest address with custom behaviour? This is a semantic subtlety that a register return type may help to address: it enforces an unambiguous technical definition; we can only concrete statically declared register objects, so the only sensible return value is the one with the highest offset.

@mandolaerik
Copy link
Contributor

Also, once we agree on a design, we also need to extend the PR with tests and docs.

@syssimics
Copy link
Contributor

Verification #13711935: ✅ pass

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.

3 participants