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

Issue #336: Added power functionality #356

Merged

Conversation

Sanchit-kumar
Copy link
Contributor

Description & Motivation

In SystemVerilog, power (a**b) is a useful function.

It would be nice to have a synthesizable Module and associated functions for generating hardware and SystemVerilog with powers as per issue #336

Testing

Added necessary test cases to test/logic_value_test.dart and test/math_test.dart

Documentation

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

Yes. Updated both README.md and doc/tutorials/chapter_2/00_basic_logic.md

@mkorbel1 mkorbel1 linked an issue May 8, 2023 that may be closed by this pull request
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Looks very good, thank you for the contribution! A few suggestions

lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
test/logic_value_test.dart Show resolved Hide resolved
lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple minor things

lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
@Sanchit-kumar
Copy link
Contributor Author

Hi @mkorbel1

There is a test case where LogicValue.ofBigInt(BigInt.zero, width) for any width > 64 leads to throw an error in case of arithmetic +,-,* operations when applied to the same LogicValue. It throws an error only when both are valued zero.

E.g., LogicValue.ofBigInt(BigInt.zero, 128) + (LogicValue.ofBigInt(BigInt.zero, 128)) throws an error.

Can you please confirm if this is an issue or I misunderstood something. If it's an issue, we need to handle _FilledLogicValue with width>64 also for arithmetic operations.

@mkorbel1
Copy link
Contributor

There is a test case where LogicValue.ofBigInt(BigInt.zero, width) for any width > 64 leads to throw an error in case of arithmetic +,-,* operations when applied to the same LogicValue. It throws an error only when both are valued zero.

E.g., LogicValue.ofBigInt(BigInt.zero, 128) + (LogicValue.ofBigInt(BigInt.zero, 128)) throws an error.

Can you please confirm if this is an issue or I misunderstood something. If it's an issue, we need to handle _FilledLogicValue with width>64 also for arithmetic operations.

@Sanchit-kumar, yes this looks like a bug, thanks for raising it! I think this is a manifestation of #299 which is WIP being addressed in PR #319 (which has been WIP for a while, should probably poke on that). Your test case that reproduces the issue is pretty simple, so I'll make a note on that issue to suggest using that as an additional test case. Sorry you ran into this!

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Changes look good! A few more things

lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
lib/src/values/logic_value.dart Outdated Show resolved Hide resolved
test/math_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for the contribution!

@mkorbel1 mkorbel1 merged commit 18dd6e9 into intel:main May 17, 2023
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.

Add power functionality
2 participants