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

Reduction module #180

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

Conversation

desmonddak
Copy link
Contributor

Description & Motivation

Modified tree reduction to create modules instead of inline assignments.

Related Issue(s)

None

Testing

ran existing tree reduction tests

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

No. API and naming match.

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

None required.

@desmonddak desmonddak requested a review from mkorbel1 February 22, 2025 18:01
@desmonddak desmonddak requested a review from mkorbel1 March 1, 2025 07:27
final Logic Function(List<Logic> inputs, {String name}) _operation;

/// Specified width of input to each reduction node (e.g., binary: radix=2)
late final int _radix;
Copy link
Contributor

Choose a reason for hiding this comment

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

primitive types like int and bool, or really any non-hardware-signal/module thing (even Functions like operation), especially when they are directly passed from the user, can be just public final variables with this.radix type of syntax in the constructor. That's also better for when you reference those variables in documentation comments [radix] vs [_radix]

Logic? clk,
Logic? enable,
Logic? reset,
super.name = 'reduction_tree'})
: super(definitionName: 'ReductionTree_R${radix}_L${sequence.length}}') {
if (sequence.isEmpty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this now support empty sequences?

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