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

42compress #142

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

42compress #142

wants to merge 18 commits into from

Conversation

desmonddak
Copy link
Contributor

Description & Motivation

Adds 4:2 compressors option to ColumnCompressor as well as a better delay table for optimizing the compression tree.

Added a popCount as a LogicValue extension which may need to be migrated to ROHD itself (along with majority). Eventually we will need hardware versions of these.

Related Issue(s)

#120 #86

Testing

Modified tests to enable the 4:2 compression option.

Backwards-compatibility

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

compression trees will take on a different shape with different delay tables as the delay optimization gets more realistic.

Documentation

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

Yes. Code is updated. We could update the component generation to expose this option as well as we need to update the documentation of multipliers that use this option.

@desmonddak desmonddak requested a review from mkorbel1 November 20, 2024 22:53
lib/src/arithmetic/addend_compressor.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/addend_compressor.dart Outdated Show resolved Hide resolved
lib/src/utils.dart Outdated Show resolved Hide resolved
@desmonddak desmonddak requested a review from mkorbel1 January 30, 2025 20:44
lib/src/arithmetic/addend_compressor.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/addend_compressor.dart Outdated Show resolved Hide resolved
/// The carry results [carry].
Logic get carry => output('carry');

late final List<List<double>> _delays;
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it may be useful to expose a @protected API for setting these delays rather than private, so that if someone has a technology-specific delay they want to set, they could just extend one of the compressors and change the delay numbers? this also leads to another question: should there be an argument to generate compressors in ColumnCompressor (even the smaller ones) rather than just a flag on whether to use 4:2 or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bare minimum is to use 2:2 and 3:2 compressors (half and full-adders). But you have a point -- people have invented even wider compressors.

Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to include changes to support arbitrary delays and user-defined compressors in this PR? one consideration is this flag to "use 4:2 compressors" is an API that wouldn't really make sense if we later replace it with a list of compressor types that can be used, for example

}

/// Compute the first One find operation on LogicValue, returning its position
int? firstOne() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could still import that other non-public file to gain access to this, rather than copy-paste the function

Copy link
Contributor

Choose a reason for hiding this comment

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

i still see it in both "evaluate_partial_product.dart" and "logicvalue_extension.dart"

test/arithmetic/multiplier_encoder_test.dart Show resolved Hide resolved
'${signedMultiplier ? 'SM_' : ''}'
'${selectSignedMultiplicand != null ? 'SSD_' : ''}'
'${selectSignedMultiplier != null ? 'SSM_' : ''}'
'with${adderGen(a, b).definitionName}') {
Copy link
Contributor

Choose a reason for hiding this comment

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

this call to adderGen i think will actually construct an adder connected to those ports, which could lead to additional verilog generation (outside of the module, even) and also slow down simulation since it's simulating a whole additional adder. i think i remember seeing this pattern in a few other places as well and didnt think it through.

/// The carry results [carry].
Logic get carry => output('carry');

late final List<List<double>> _delays;
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to include changes to support arbitrary delays and user-defined compressors in this PR? one consideration is this flag to "use 4:2 compressors" is an API that wouldn't really make sense if we later replace it with a list of compressor types that can be used, for example

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.

ColumnCompressor should be support more compressors like 4:2
3 participants