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

Avoid Module creation for Constant Gate scenarios #481

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

Conversation

mjayasim9
Copy link
Contributor

Description & Motivation

#429

Related Issue(s)

#429

Testing

Added test cases to check if the result of '&', '|' and '~' operations are as expected for constant input scenarios.

Backwards-compatibility

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

No.

Documentation

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

No.

Logic operator ~() => NotGate(this).out;
Logic operator ~() {
if (this is Const) {
if (value.isValid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can rely on the operators within LogicValue for this? i.e. something like Const(~this.value)?

return this;
}
} else {
return Const(LogicValue.x, width: width);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right, consider 11x11 & 11111 == 11x11

If we just cover the cases of all 0's and all 1's that's probably enough and let the rest fall through to the and gate?

other.value == LogicValue.of(0, width: width)) {
return Const(LogicValue.of(0, width: width));
} else if (value == LogicValue.filled(width, LogicValue.one)) {
return other;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if other is Const and this is not Const but has a value of all 1s currently? Need to be cautious to only consider the value if it's a Const.

@mkorbel1 mkorbel1 linked an issue Apr 5, 2024 that may be closed by this pull request

/// Logical bitwise AND.
Logic operator &(Logic other) => And2Gate(this, other).out;
Logic operator &(Logic other) {
if (this is Const &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made a little more general? If either one is a Const, then you can do something special:

  • If one of the two is constant all 1's, then return the other
  • If one of the two is constant all 0's, then return 0

The current logic only covers if both are const.

You could remove some duplication in the implementation by making local variables to point to "the constant one" and "the other one", so you don't need to consider it both ways for all cases.


/// Logical bitwise OR.
Logic operator |(Logic other) => Or2Gate(this, other).out;
Logic operator |(Logic other) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here for or

@@ -353,6 +353,115 @@ void main() {
});
});

group('Const input Gate test', () {
test('NotGate single bit Constant input', () async {
final a = Logic();
Copy link
Contributor

Choose a reason for hiding this comment

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

naming the variables could be helpful for understanding the intent of the test, e.g. normalLogic, const0, etc.

final d = Const(LogicValue.one, width: 1);
a.put(LogicValue.one);

expect(a & b, isA<Logic>());
Copy link
Contributor

Choose a reason for hiding this comment

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

anything & 0 should be a constant 0

expect((a | c).value, equals(LogicValue.one));

expect(a | d, isA<Logic>());
expect((a | d).value, equals(LogicValue.one));
Copy link
Contributor

Choose a reason for hiding this comment

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

anything | 1 should be 1

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.

Avoid module creation for simple constant scenarios in gates
2 participants