Provide an implementation of litOption() for BundleLits #1280

ucbjrl · 2019-12-18T00:20:45Z

Related issue: #1277, ucb-bar/dsptools#184

Type of change: bug report

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes
Provide an implementation of litOption for Aggregates (Bundles).

This is just a band aid until an Aggregate `isLit()` method (for which work has begun) is implemented.

# Conflicts: # chiselFrontend/src/main/scala/chisel3/Aggregate.scala

jackkoenig · 2019-12-18T00:23:06Z

Probably want to rebase/cherry-pick on master since bandaid None was squashed-and-merged

ucbjrl · 2019-12-18T00:23:53Z

We should add some tests to verify this works with Vec literals as well.

ducky64

Some style concerns, a question, and a possible overflow, but otherwise looks sane.
Also consider adding a test for a literal with a >64 bit element.

ducky64 · 2019-12-18T08:52:27Z

chiselFrontend/src/main/scala/chisel3/Aggregate.scala

+      val value = shiftadd._2 & mask
+      (accumulator << width) + value
+    }
+    try {


The need for a try-catch seems like a red flag. What is this meant to catch that isn't handled by an Option? And can we fix that instead?

ducky64 · 2019-12-18T08:55:17Z

chiselFrontend/src/main/scala/chisel3/Aggregate.scala

+    try {
+      topBindingOpt match {
+        case Some(BundleLitBinding(_)) =>
+          Some(getElements.reverse.map{ case e => (e.width.get, e.litOption().get)}.foldLeft(BigInt(0))(shiftAdd(_, _)))


I think the map and fold should be broken into multiple lines for readability. Took a while to parse this...

ducky64 · 2019-12-18T08:56:06Z

chiselFrontend/src/main/scala/chisel3/Aggregate.scala

+    // Shift the accumulated value by our width and add in our component, masked by our width.
+    def shiftAdd(accumulator: BigInt, shiftadd: (Int, BigInt)): BigInt = {
+      val width = shiftadd._1
+      val mask = (1 << width) - 1


Is this a Int or BigInt type? Will this work for >32 bits?

ducky64 · 2019-12-18T08:57:27Z

chiselFrontend/src/main/scala/chisel3/Aggregate.scala

+    def shiftAdd(accumulator: BigInt, shiftadd: (Int, BigInt)): BigInt = {
+      val width = shiftadd._1
+      val mask = (1 << width) - 1
+      val value = shiftadd._2 & mask


Is the mask ever necessary? I'd be tempted to assert out of the mask actually did anything, since that means a literal is overflowing its allotted width?

ducky64 · 2019-12-18T09:01:01Z

src/test/scala/chiselTests/BundleLiteralSpec.scala

@@ -28,7 +28,8 @@ class BundleLiteralSpec extends ChiselFlatSpec {
      chisel3.assert(outsideBundleLit.a === 42.U)
      chisel3.assert(outsideBundleLit.b === true.B)
      chisel3.assert(outsideBundleLit.c === MyEnum.sB)
-
+      chisel3.assert(outsideBundleLit.isLit())
+      chisel3.assert(outsideBundleLit.litValue().U === outsideBundleLit.asUInt())


Is there a literal packing test anywhere? Do we want to test that too, by equivalence with a "magic number" in the test case, instead of solely equivalence with the .asUInt behavior?

I think LiteralExtractorSpec provides Lit test for Element type, and tests for Lit.
But in my understanding, Lit and LitOption are two different thing:
Lit will return a Option[BigInt], while LitOption will return T itself.
As for this case:
I think the result of outsideBundleLit.litValue().U === outsideBundleLit.asUInt() is only a necessary condition to this test.

If we wanna fully test this, I think the we should test the behavior of firrtl behavior of chisel3.internal.firrtl.PrimOp.AsUIntOp is equal to the (accumulator << width) + masked) behavior. At my first glance to firrtl.PrimOps.AsUInt, this behavior is correct. I think this is good enough here.

ducky64 · 2020-06-29T04:45:32Z

Updated PR:

merged in latest
got rid of the try block, in favor of using Option throughout the foldLeft
added packing tests, including with signed and fixed-point types

It seems to work.

But one more general concerning point: in the LongBundle literal specifier, if I don't have explicit widths for the components (not in the most-significant position, at least), it seems .asUInt will return a different value in simulation. So it seems the structural width of the bundle is being ignored for the widths of literal elements in the simulation bundle packing, which seems wrong?

PR ownership transfer

sequencer

Generally, lgtm expect some small nitpicking.

core/src/main/scala/chisel3/Aggregate.scala

src/test/scala/chiselTests/BundleLiteralSpec.scala

sequencer · 2020-06-30T17:42:59Z

src/test/scala/chiselTests/BundleLiteralSpec.scala

@@ -28,7 +28,8 @@ class BundleLiteralSpec extends ChiselFlatSpec {
      chisel3.assert(outsideBundleLit.a === 42.U)
      chisel3.assert(outsideBundleLit.b === true.B)
      chisel3.assert(outsideBundleLit.c === MyEnum.sB)
-
+      chisel3.assert(outsideBundleLit.isLit())
+      chisel3.assert(outsideBundleLit.litValue().U === outsideBundleLit.asUInt())


I think LiteralExtractorSpec provides Lit test for Element type, and tests for Lit.
But in my understanding, Lit and LitOption are two different thing:
Lit will return a Option[BigInt], while LitOption will return T itself.
As for this case:
I think the result of outsideBundleLit.litValue().U === outsideBundleLit.asUInt() is only a necessary condition to this test.

If we wanna fully test this, I think the we should test the behavior of firrtl behavior of chisel3.internal.firrtl.PrimOp.AsUIntOp is equal to the (accumulator << width) + masked) behavior. At my first glance to firrtl.PrimOps.AsUInt, this behavior is correct. I think this is good enough here.

ducky64 · 2020-07-06T18:32:54Z

Fixed stylistic issues mentioned in review comments.

Not sure why GitHub isn't allowing a response to the last comment but:

I'm not sure I completely understand what you mean. LiteralExtractorSpec does test litValue and litOption, but doesn't have tests for bundles. I think an argument could be made for the bundle litOption tests to go in LiteralExtractorSpec, but I like centralizing all the bundle literal operations (including extraction) in a bundle literal test file.
I don't understand what you with Lit vs. LitOption. In the Data base class, litValue just delegates to litOption, so testing either makes sense?
For outsideBundleLit.litValue().U === outsideBundleLit.asUInt(), this should test FIRRTL behavior since it goes through to compilation and simulation. It's a blackbox test of FIRRTL since it only checks the overall simulation result, but I think that makes sense for this test.

sequencer

As Jack mentioned in dev meeting, chisel3.assert will invoke FIRRTL to test, so generally lgtm. Thanks @ducky64

ucbjrl added 2 commits December 17, 2019 09:20

Band aid until litOption is implemented for Aggregates.

b3316a5

This is just a band aid until an Aggregate `isLit()` method (for which work has begun) is implemented.

Implement litOption for Aggregates (Bundles)

ae19c10

ucbjrl requested a review from a team as a code owner December 18, 2019 00:20

jackkoenig requested a review from ducky64 December 18, 2019 00:22

Merge branch 'master' into bundlelitoptionfix

f8ac286

# Conflicts: # chiselFrontend/src/main/scala/chisel3/Aggregate.scala

ducky64 previously requested changes Dec 18, 2019

View reviewed changes

ducky64 added 4 commits June 28, 2020 17:33

Merge branch 'master' into bundlelitoptionfix

0dbb043

add unpack tests, add long test

23712a1

clean up error case

a773cbf

add longer tests w/ signed and fixed types

494e1a6

sequencer requested changes Jun 30, 2020

View reviewed changes

ducky64 and others added 2 commits July 6, 2020 11:21

address review comments

b994106

Merge branch 'master' into bundlelitoptionfix

73e8793

sequencer approved these changes Jul 9, 2020

View reviewed changes

ducky64 mentioned this pull request Jul 13, 2020

Bundle asUInt ignores structural width #1510

Open

seldridge approved these changes Jul 13, 2020

View reviewed changes

ducky64 merged commit 417fc2a into master Jul 13, 2020

ducky64 deleted the bundlelitoptionfix branch July 13, 2020 18:26

ducky64 mentioned this pull request Aug 7, 2020

Add peek/poke/expect for memories ucb-bar/chiseltest#182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide an implementation of litOption() for BundleLits #1280

Provide an implementation of litOption() for BundleLits #1280

ucbjrl commented Dec 18, 2019

jackkoenig commented Dec 18, 2019 •

edited

Loading

ucbjrl commented Dec 18, 2019

ducky64 left a comment

ducky64 Dec 18, 2019

ducky64 Dec 18, 2019

ducky64 Dec 18, 2019

ducky64 Dec 18, 2019

ducky64 Dec 18, 2019

sequencer Jun 30, 2020

ducky64 commented Jun 29, 2020

sequencer left a comment

sequencer Jun 30, 2020

ducky64 commented Jul 6, 2020

sequencer left a comment

Provide an implementation of litOption() for BundleLits #1280

Provide an implementation of litOption() for BundleLits #1280

Conversation

ucbjrl commented Dec 18, 2019

jackkoenig commented Dec 18, 2019 • edited Loading

ucbjrl commented Dec 18, 2019

ducky64 left a comment

Choose a reason for hiding this comment

ducky64 Dec 18, 2019

Choose a reason for hiding this comment

ducky64 Dec 18, 2019

Choose a reason for hiding this comment

ducky64 Dec 18, 2019

Choose a reason for hiding this comment

ducky64 Dec 18, 2019

Choose a reason for hiding this comment

ducky64 Dec 18, 2019

Choose a reason for hiding this comment

sequencer Jun 30, 2020

Choose a reason for hiding this comment

ducky64 commented Jun 29, 2020

sequencer left a comment

Choose a reason for hiding this comment

sequencer Jun 30, 2020

Choose a reason for hiding this comment

ducky64 commented Jul 6, 2020

sequencer left a comment

Choose a reason for hiding this comment

jackkoenig commented Dec 18, 2019 •

edited

Loading