-
Notifications
You must be signed in to change notification settings - Fork 33
pushed dynamic configuration through all multiplier componentry #222
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
base: main
Are you sure you want to change the base?
Conversation
|
It would be nice to generalize this to integer or boolean configurations. A simple test example I can use is a shift register (or the rotater) which would take either a Logic or an int for a shift input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great!
| /// The actual shift in each row. This value will be modified by the | ||
| /// sign extension routine used when folding in a sign bit from another | ||
| /// row. | ||
| /// row. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: accidentally added duplicate "row." here?
| throw RohdHclException('multiplicand sign reconfiguration requires ' | ||
| 'signedMultiplicand=false'); | ||
| } | ||
| // if (signedMultiplicand && (selectSignedMultiplicand != null)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm old code
| throw RohdHclException('multiplicand sign reconfiguration requires ' | ||
| 'signedMultiplicand=false'); | ||
| } | ||
| // if (signedMultiplicand && (selectSignedMultiplicand != null)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm old code
| final Logic sign; | ||
| if (selectSignedMultiplicand != null) { | ||
| sign = mux(selectSignedMultiplicand!, addend.last, signs[row]); | ||
| if (signedMultiplicandParameter.dynamicConfig != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice a lot of checking for != null, would be nice to have a bool get isDynamic helper?
|
|
||
| /// Factory constructor to create a [StaticOrDynamicParameter] instance from a | ||
| /// dynamic. | ||
| factory StaticOrDynamicParameter.ofDynamic(dynamic config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this the most commonly used constructor, yet the name is the longest and also a little weird (though I dont have a better suggestion, it seems appropriate) since "dynamic" is used in two meanings in the same call: a changing Logic and a dynamically typed variable.
What if we made this constructor just StaticOrDynamicParameter and the other one something like StaticOrDynamicParameter.withConfigs or something (maybe a better name)?
| if (selectSignedMultiplicand != null) { | ||
| sign = mux(selectSignedMultiplicand!, addend.last, signs[row]); | ||
| if (signedMultiplicandParameter.dynamicConfig != null) { | ||
| sign = mux(signedMultiplicandParameter.dynamicConfig!, addend.last, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we added a mux or select function into this StaticOrDynamicParameter that automatically handles the switch between using a mux or a ternary operator? this would only work when it's a 1-bit value.
also, mux will be updated (eventually) so that if the select is a Const, it will resolve the result statically. but it still might be worth adding it as an option on this infrastructure
| } | ||
| this.subtractIn = | ||
| (subtractIn != null) ? addInput('subtractIn', subtractIn) : null; | ||
| subtractIn = StaticOrDynamicParameter.ofDynamic(subtract).clone(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why clone?
Description & Motivation
Completed the use of dynamic configuration of signed operands through the multiplier componentry.
Related Issue(s)
None.
Testing
Ran existing multiplier tests.
Backwards-compatibility
Yes. Various components now have simpler APIs for sign configuration of multiplicand and multiplier (and addend for MAC).
Documentation
API documentation fully updated. Markdown documentation TBD.