BitSetRange API

[CircuitCoder]

Add a new implementation of chisel3.util.experimental.BitSet, BitSetRange, matching continuous bit ranges. Use cases include representing address ranges.

This is currently a WIP.
TODO: A matches method is added to all BitSet for testing, which currently has a naive implementation. We need to change that to a better implementation (e.g. using decoder)
TODO: Maybe add a BitSet.range as an alias of BitSetRange.apply?
TODO: Maybe add a overload BitSetRange.apply(Range) to permit code such as BitSetRange(0x1000 until 0x5000)
TODO: Documentations are not finished.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

new API

API Impact

Add a new class BitSetRange representing BitSet corresponding to continuous bit ranges.
Add a new object BitSetRange for constructing BitSetRange instances.

Backend Code Generation Impact

Desired Merge Strategy

Squash

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[sequencer]

This needs some ScalaDoc to explain the BitSetRange

[CircuitCoder]

Yes I'll add them after the API goes through discussion.

[sequencer]

Generally LGTM, would you mind have a try to copy-paste this PR to RocketChip? replacing its address decoder, to see if it can pass the rocket-chip CI.

[CircuitCoder]

Thoughts on converting from generic BitSet to BitSetRange:

We can also convert ANY BitSet into a list of continuous ranges. The problem is that the number of ranges maybe very large (e.g. BitPat("???????????????0")). So an API similar to BitSet.toRanges: Seq[BitSetRange] may take ages.

An alternative is BitSet.toRanges: Iterator[BitSetRange], then the potentially large number of ranges is the API users' problem. They can choose to handle it when working with dynamic data, or just collect into a Seq when they are sure there are not many regions.

The steps to generate each range is as follows:

• Calculate the lower bound of the BitSet, which is quite easy to do: fill in 0 into ? in all terms, and pick the smallest one. Let l be that lower bound.
• Calculate the lower bound of complement of (the original bitset unions [0, l)). Let that be r.
• [l, r) is the lowest continuous region. Subtract that region from the bitset in the iterator.

Another idea is add a maxRegionNumber argument. But I personally prefers the one using iterators, as it provides more flexibility.

[sequencer]

Here is my thought:

1. we do need a BitSetRange type, semantic of which is: "a continues BitSet", here are two reasons:

• for matches function, Compare can provide better PPA since it leverage the adder rather than pure PLA to match on signals.
• toString function can be overridden to print the Range rather than the confusing truth table.

2. Generally, I like the Iterator idea.

• It's a good idea to throw the NPC problem to users.
• only consuming a part of BitSet is irrational, for most of usages, all converted data is needed. Whether this to be a user API need to be discussed, I think it need to be decided by @jackkoenig.
• another propose is having another API: toBitSetRange(giveUpAt: Int = someReasonableThreshold): Seq[BitSetRange], this API should use the Iterator to generate and finally return a Seq of BitSetRange or throw an error.

[CircuitCoder]

@sequencer An initial version of BitSet.toRanges is implemented in c8013dd. May requires additional perfing, but since the BitSet within the iterator should never increase in term count, it should mostly be fine.

I'm working on providing some sort of randomized testing.

[sequencer]

It may run forever, but this API is necessary, I think the time complexity may need to be documented? or at least tell users this API is time consuming.

[CircuitCoder]

This is just a simple reduction into BitSet.subtract. If I'm not mistaken, the maximum number of generated terms is width * len(term), which doesn't seems that bad ™️

Actually, if we are to document about the complexity, we may also want to document about the complexity of BitSet.subtract?

[sequencer]

yes, basically all complexity should come from subtract. emmm. that's time consuming as well. I think mention it on ScalaDoc is good enough yet.

[CircuitCoder]

Tests are added in 6f079f7.

Actually I encountered an unexpected result from BitSet.getWidth. An empty BitSet returns zero for its width, but the BitSet may came from subtracting two BitSets with positive width. One would expect whenever I subtract another BitSet won't change one BitSet's width.

@sequencer Is this considered a bug, or a expected behavior? If this is indeed unexpected, then we need to keep a separate record of one BitSet's width. Also, BitSet.empty need to take an width argument (or we can keep a special value (e.g. -1) as a wildcard, and throw an exception on getWidth?)

Besides, are we going to allow binary subtraction / union / intersection between BitSets with different width? If we do, then how are we going to pad the shorter one?

[jackkoenig]

I'm going to try to summarize a really good discussion we had in Chisel dev today (thanks to @sequencer and @seldridge for the great session!).

I'm going to use the term AddressSet instead of BitSetRange for purposes of this writeup for some clarity but just note they refer to the same idea.

With the current implementation, it does not make sense to have AddressSet extend BitSet because the underlying primitive (and unit of reuse) of BitSet is terms of type Set[BitPat]. The entire purpose of AddressSet is that some BitSet-like operations can be done more efficiently on that representation than they can be on the equivalent Set[BitPat] representation. But this benefit is thrown away whenever we do an operation that converts the AddressSet to Set[BitPat], more-or-less defeating the purpose.

That being said, I do see the utility in being able to do something like take an AddressSet, union it with a BitSet and then check if a UInt matches the union. I see 3 options:

1. BitSet and AddressSet are separate APIs (no inheritance relationship)

This is the simplest to implement and saves us from having to make difficult implementation decisions that will be discussed in the later options. This does mean that users will have to handle any composition of AddressSet with BitSet themselves. For example, if checking that a UInt matches the union of an AddressSet and a BitSet, a user will have to check the UInt against each and then || the result.

2. BitSet and AddressSet implement a common trait (HWSet? ChiselSet?)

Similarly how we introduced BitSet to make it possible to represent certain operations on BitPat that cannot be represented with a single BitPat, we need the ability to represent this operations on the combination of BitSets and AddressSets. The reason for this new type is that the BitSet primitive being Set[BitPat] does not adequately capture such combinations (it can capture it but you get bad circuits as your outputs). Thus we can have other concrete subtypes of this new type called things like Union, Intersection, and Subtraction (or Difference).

Thus, the union of an AddressSet and a BitSet will return a Union. This saves the higher-level user intent so that later uses of this Union (eg. decoding or checking if a UInt matches it) can still be implemented efficiently. Note that this does not mean we always return a Union when you union two BitSet, we can continue to use more optimal solutions whenever combining just BitPats together.

Furthermore, this has highlighted to me that the current Set[BitPat] datastructure in BitSet is sort of a "normalized form" (it looks sort of like disjunctive normal form if you squint but perhaps BitPat is the real DNF). My real argument here is that when combining AddressSet with BitSet we do not want to normalize because we will get a worse circuit if we do.

3. AddressSet extends BitSet but we deprecate and remove the terms: Set[BitPat] method

This one is basically the same in every way as (2), except that instead of introducing a new supertype (that we have to come up with a good name for), we just reclaim BitSet as the super type.

Originally this was my favorite option but with my recent realization that BitSet being a Set[BitPat] looks very similar to a normalized form (although may not be) makes me question if we should keep the Set[BitPat] representation around. I'm not really sure here though because we could just create a new concrete subtype of BitSet that captures Set[BitPat].

[CircuitCoder]

After some discussion with @sequencer (thanks for his patience since I had so much misunderstanding), now I really like the idea of the common trait approach. If I'm understanding correctly, this approach effectively decouples the construction, optimization, and consumption of a BitSet. Operations on BitSets become more similar to operations on immutable data structures, where we construct a tree without premature normalization/optimization. Optimization is the transformation between equivalent trees, and consumption has the semantics of simple recursions on trees.

If that's what we end up doing, I believe the current BitSet is the same as the union of some BitPats? So I guess (3) will work just fine. BitPat and AddressSet will be the leaf nodes in the tree and differs in the way we consume them (with pattern match or with comparison).

Some other thoughts on what to do now:

The original motivation behind the API is that there lacks a way to directly construct a BitSet matching ranges. How we can optimize the consumption of such ranges is sort of an afterthought. So an API like BitSet.fromRanges may fit this purpose better. This is reflected in the current implementation: BitSetRange is merely a type-level guarantee that a BitSet is continuous. Then now we have this extra information, we can do a little bit of optimization.

Since some library features (crossbar, router, etc) are blocked by the lack of a way to construct such BitSet, maybe we can add that feature first? Some options are:

• Remove BitSetRange entirely, move current BitSetRange.terms to BitSet.fromRange. Continous BitSets are immediately broken into BitPats during construction. This is the bare minimum implementation of such API, and discards any optimization opportunity until we switch to the new approach.
• Keep BitSetRange, maybe allow more optimization, e.g.:
  • Check if results are continuous in BitSet operations (intersect, union, etc.). If they are, return the corresponding continuous version instead.
  • Add methods like BitSetRange.intercsectRange(BitSetRange): BitSetRange, BitSetRange.unionRange(BitSetRange): Maybe[BitSetRange].
• Just switch to the tree approach right away. The downside is that right now optimizations and consumptions have to be implemented in the frontend, so optimizations can only follow simple heuristics.

[jackkoenig]

We've discussed this in the developers' meeting. In order to enable development without having to be 100% certain on the API, we should merge this to master and not backport to 3.5.x. This means we can iterate a bit using the SNAPSHOTs and see how we feel about the API.

@CircuitCoder

if I'm understanding correctly, this approach effectively decouples the construction, optimization, and consumption of a BitSet. Operations on BitSets become more similar to operations on immutable data structures, where we construct a tree without premature normalization/optimization. Optimization is the transformation between equivalent trees, and consumption has the semantics of simple recursions on trees.

If that's what we end up doing, I believe the current BitSet is the same as the union of some BitPats? So I guess (3) will work just fine. BitPat and AddressSet will be the leaf nodes in the tree and differs in the way we consume them (with pattern match or with comparison).

Your understanding here is 100% correct, so as you say, it's really just how should we proceed.

I'm totally fine with merging some way of doing this even if it blasts down to Set[BitPat] and we figure out the optimization approach later. I think BitSet.fromRange is the most pragmatic approach to get something working right away so I am fine with it. My only concern is that because the API necessarily constructs a BitSet (and BitSet has terms: Set[BitPat]) we will either need to change this API before release of Chisel 3.6 or deprecate terms: Set[BitPat]. We don't have to make this decision right now, I am more than happy to approve something just to get the ball rolling.

I'm not super keen on keeping BitSetRange but having special handling to allow more optimization. If we're going to allow more optimization, we should switch to the tree approach. I think it's totally fine to add BitSet.fromRange (lacking optimization) for now, and then go straight to the tree implementation. While I agree that it will be hard to do as good of a job at the moment just in the frontend, I think that the heuristics we would follow would actually be pretty decent in practice and still leave the door open for direct IR support in the future and a better overall implementation. The neat thing about the tree approach (even if implemented in the frontend) is that a later IR implementation can be substituted in without any API changes.

TLDR Let's go ahead with merging BitSet.fromRange to master so we can play with the API, and we can discuss how we want to eventually implement the more complete, tree-based version.

[sequencer]

@CircuitCoder any updates on this PR?

[CircuitCoder]

Just moved all logics from BitSetRange into BitSet.fromRange in 2aefb34. Thie PR should be ready to be merged!

[sequencer]

generally LGTM, ask @jackkoenig to review again.
In 3.7, we should move theBitSet manipulation logic into MFC,
but currently, we can just use this to create new circuit related to interconnection.

[jackkoenig]

I've left some nitty comments and an API suggestion (I don't like using -1 as an indicator of "width not specified"). I'm not going to request changes because I will be gone for the next 2 weeks but I hope my suggestions are addressed before merging.