Allow a counter to be instantiated using a Scala range #1515

nullobject · 2020-07-15T09:30:42Z

This PR adds the ability for a counter to be instantiated using a Scala range.

The original API has been preserved, but the counter now works internally with a range. Using a range to configure the counter allows us to do interesting things.

Counting upwards:

val (value, wrap) = Counter(0 to 3)
// 0, 1, 2, 3, 0, 1...

Counting downwards:

val (value, wrap) = Counter(3 to 0)
// 3, 2, 1, 0, 3, 2...

Counting upwards by twos:

val (value, wrap) = Counter(0 to 9 by 2)
// 0, 2, 4, 6, 8, 0, 2...

It also allows an optional condition attribute (similar to the existing API):

val enable = true.B
val (value, wrap) = Counter(0 to 3, enable)
// 0, 1, 2, 3, 0, 1...

I'm fairly new to Chisel, but I think that having the condition as the second parameter feels more consistent with the rest of the Chisel API.

I'm happy to scale this PR back if you think it's changing too much at once, but I'm keen to get some feedback on the overall goal. Thoughts?

Type of change: feature request

Impact: API addition (no impact on existing code)

Development Phase: proposal

Release Notes
Allow a counter to be instantiated using a Scala range

CLAassistant · 2020-07-15T09:30:49Z

All committers have signed the CLA.

src/main/scala/chisel3/util/Counter.scala

jackkoenig

This is super cool and I love being able to create Counters from Ranges, thanks!

A couple of issues I highlighted in a comment:

I think we should keep the Verilog the same for the simple case 0 until power-of-2
In at least that case, the width of the register seems to be 1 bigger than it needs to be

src/main/scala/chisel3/util/Counter.scala

aswaterman · 2020-07-16T21:57:51Z

src/main/scala/chisel3/util/Counter.scala

+      val wrap = value === r.last.U
+
+      when (wrap) {
+        value := r.start.U


I think the special case can be applied by sticking an if (!(isPow2(r.end) && isPow2(r.step))) here.

Thanks. I ended up using something very similar to that conditional, but had to also check that the range starts at zero.

nullobject · 2020-07-21T09:45:32Z

@jackkoenig @aswaterman Please re-review when you get a chance.

I'm happy to keep tweaking this until you guys are happy 😸

src/main/scala/chisel3/util/Counter.scala

jackkoenig

I can confirm the code gen for 0 until power-of-2 case is resolve, thank you for that! This LGTM, some suggestions for improvement but they're not necessary for this PR. Thanks!

src/main/scala/chisel3/util/Counter.scala

jackkoenig · 2020-07-23T19:27:34Z

src/main/scala/chisel3/util/Counter.scala

+  def apply(r: Range, cond: Bool = true.B): (UInt, Bool) = {
+    val c = new Counter(r)
+    val wrap = WireInit(false.B)
+    when (cond) { wrap := c.inc() }
+    (c.value, wrap)


This is a minor API question, but the range API is a little inconsistent with the old API, if you want an enable its:

Counter(0 until 16, en) // vs. Counter(en, 16)

I won't contest that maybe it's the existing API that's awkward, but we probably ought to be consistent here

@jackkoenig Thanks for your thoughts 🤔

Unfortunately, flipping the order of the params would make having default values for the enable param more awkward. IMHO, since you can't construct a counter without a range value (i.e. what good is a counter with an enable and no range?), it makes sense that this is the first parameter.

My vote would be to deprecate the old API. That way users could have:

Counter(10) Counter(10, en) Counter(0 to 10) Counter(0 to 10, en)

I'm happy to open a new PR to discuss?

jackkoenig · 2020-07-23T19:38:09Z

@chick or something with admin rights, can you check why the CI isn't reporting?

jackkoenig · 2020-07-24T18:54:43Z

@nullobject the test failure may be a real one, the tests that failed use Counter

nullobject · 2020-07-27T12:00:31Z

the test failure may be a real one, the tests that failed use Counter

@jackkoenig Fixed. It was due to a counter being instantiated with Counter(0) in the shift register test. This was creating an empty range.

I have update the legacy constructor to always create a non-empty range, and added an assertion to ensure that counters can never be instantiated with an empty range.

I don't think supporting empty ranges makes much sense, because how can you ever count zero elements?

aswaterman · 2020-07-27T23:02:49Z

src/main/scala/chisel3/util/Counter.scala

+  require(r.start >= 0 && r.end >= 0, s"Counter range must be positive, got: $r")
+
+  private lazy val delta = math.abs(r.step)
+  private lazy val width = math.max(log2Up(r.last + 1), log2Up(r.head + 1))


Not a problem, but just FYI: abs and max are defined on the various integral types, so you can write e.g. r.step.abs or foo max bar.

Thanks 😄

I'm still learning how to write idomatic Scala.

aswaterman · 2020-07-27T23:05:08Z

I think updating the legacy constructor to support Counter(0) is fine. I think it makes sense that the range constructor should require a nonempty range since it represents a half-open interval.

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

We only need to explicitly wrap counters that don't start at zero, or end on a power of two. Otherwise we just let the counter overflow naturally to avoid wasting an extra mux.

jackkoenig · 2020-07-30T04:36:38Z

@nullobject do you care how this is merged? Do you want it with a merge commit or is a squash-and-merge fine?

Also sorry for the delay, somehow I missed the last few emails about this PR.

nullobject · 2020-07-30T04:38:41Z

@nullobject do you care how this is merged? Do you want it with a merge commit or is a squash-and-merge fine?

Also sorry for the delay, somehow I missed the last few emails about this PR.

@jackkoenig Not really, whatever you guys normally do.

Happy to squash it all into one commit if you prefer?

jackkoenig · 2020-07-30T04:41:47Z

@nullobject since we require PRs to be up-to-date to merge, we usually usually let the trust bot @Mergify.io queue them up, merging master, then squash-and-merging. I'll get that started now. Thanks!

nullobject · 2020-07-30T04:47:09Z

@jackkoenig Fantastic. Thanks for persisting with my PR.

I'm happy to have made my first contribution to Chisel 🙌

jackkoenig · 2020-07-30T05:15:03Z

@nullobject Thank you so much! It was a pleasure and an excellent contribution!

nullobject force-pushed the counter-range branch 2 times, most recently from 86e9a12 to d83f54e Compare July 15, 2020 09:45

nullobject commented Jul 15, 2020

View reviewed changes

src/main/scala/chisel3/util/Counter.scala Show resolved Hide resolved

nullobject force-pushed the counter-range branch from d83f54e to f04b177 Compare July 15, 2020 09:49

nullobject marked this pull request as ready for review July 15, 2020 09:50

nullobject requested a review from a team as a code owner July 15, 2020 09:50

nullobject changed the title ~~Allow the Counter module to be instantiated with a Scala range~~ Allow a counter to be instantiated using a Scala range Jul 15, 2020

jackkoenig requested changes Jul 16, 2020

View reviewed changes

jackkoenig reviewed Jul 16, 2020

View reviewed changes

src/main/scala/chisel3/util/Counter.scala Outdated Show resolved Hide resolved

aswaterman reviewed Jul 16, 2020

View reviewed changes

nullobject requested review from jackkoenig and aswaterman July 18, 2020 02:54

aswaterman reviewed Jul 23, 2020

View reviewed changes

src/main/scala/chisel3/util/Counter.scala Show resolved Hide resolved

jackkoenig approved these changes Jul 23, 2020

View reviewed changes

src/main/scala/chisel3/util/Counter.scala Show resolved Hide resolved

src/main/scala/chisel3/util/Counter.scala Show resolved Hide resolved

jackkoenig reviewed Jul 23, 2020

View reviewed changes

nullobject force-pushed the counter-range branch from bedb084 to b0a426a Compare July 27, 2020 11:30

nullobject requested review from jackkoenig and aswaterman July 27, 2020 23:01

aswaterman reviewed Jul 27, 2020

View reviewed changes

nullobject mentioned this pull request Jul 29, 2020

Allow a counter to be manually reset #1527

Merged

1 task

nullobject force-pushed the counter-range branch 2 times, most recently from 0fc6c93 to a70d3fa Compare July 29, 2020 10:17

nullobject added 2 commits July 30, 2020 09:20

Add positive range generator

7940c53

Allow the Counter module to be instantiated with a Scala range

8fef104

nullobject and others added 3 commits July 30, 2020 09:20

Use head/last to determine counter width

fbc7caa

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

Let counter overflow naturally when appropriate

25066ee

We only need to explicitly wrap counters that don't start at zero, or end on a power of two. Otherwise we just let the counter overflow naturally to avoid wasting an extra mux.

Require counter range to be non-empty

bded72a

nullobject force-pushed the counter-range branch from a70d3fa to bded72a Compare July 29, 2020 23:21

jackkoenig added Feature New feature, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. labels Jul 30, 2020

jackkoenig added this to the 3.4.0 milestone Jul 30, 2020

Merge branch 'master' into counter-range

c7626d9

mergify bot merged commit fca89f6 into chipsalliance:master Jul 30, 2020

nullobject deleted the counter-range branch July 30, 2020 05:15

jackkoenig mentioned this pull request Aug 11, 2020

Restore Counter.n API #1546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a counter to be instantiated using a Scala range #1515

Allow a counter to be instantiated using a Scala range #1515

nullobject commented Jul 15, 2020 •

edited

CLAassistant commented Jul 15, 2020 •

edited

jackkoenig left a comment

aswaterman Jul 16, 2020

nullobject Jul 18, 2020

nullobject commented Jul 21, 2020

jackkoenig left a comment

jackkoenig Jul 23, 2020

nullobject Jul 24, 2020

jackkoenig commented Jul 23, 2020

jackkoenig commented Jul 24, 2020

nullobject commented Jul 27, 2020

aswaterman Jul 27, 2020

nullobject Jul 27, 2020

aswaterman commented Jul 27, 2020

jackkoenig commented Jul 30, 2020

nullobject commented Jul 30, 2020

jackkoenig commented Jul 30, 2020

nullobject commented Jul 30, 2020

jackkoenig commented Jul 30, 2020

Allow a counter to be instantiated using a Scala range #1515

Allow a counter to be instantiated using a Scala range #1515

Conversation

nullobject commented Jul 15, 2020 • edited

CLAassistant commented Jul 15, 2020 • edited

jackkoenig left a comment

Choose a reason for hiding this comment

aswaterman Jul 16, 2020

Choose a reason for hiding this comment

nullobject Jul 18, 2020

Choose a reason for hiding this comment

nullobject commented Jul 21, 2020

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig Jul 23, 2020

Choose a reason for hiding this comment

nullobject Jul 24, 2020

Choose a reason for hiding this comment

jackkoenig commented Jul 23, 2020

jackkoenig commented Jul 24, 2020

nullobject commented Jul 27, 2020

aswaterman Jul 27, 2020

Choose a reason for hiding this comment

nullobject Jul 27, 2020

Choose a reason for hiding this comment

aswaterman commented Jul 27, 2020

jackkoenig commented Jul 30, 2020

nullobject commented Jul 30, 2020

jackkoenig commented Jul 30, 2020

nullobject commented Jul 30, 2020

jackkoenig commented Jul 30, 2020

nullobject commented Jul 15, 2020 •

edited

CLAassistant commented Jul 15, 2020 •

edited