BitSet API

[sequencer]

reimplement #2126, BitSet is a set of BitPat, and BitPat will extend from BitSet since essentially it is a subset of BitSet.

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 feature/API

API Impact

API modification for BitPat(width=)(ask @jackkoenig for better solution to avoid this)

Backend Code Generation Impact

None

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Add BitSet API.

Reviewer Checklist (only modified by reviewer)

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

[sequencer]

ready for @jackkoenig's review.

[sequencer]

poke @jackkoenig :)

[jackkoenig]

I know I'm being a real stickler here, but this feels like a very foundational API so I'm just trying to make sure we get it right.

[jackkoenig]

Suggested change

	/**
	* Subtract that from this BitPat
	* @param that subtrahend
	*/
	/** Subtract that from this BitPat
	*
	* @param that subtrahend
	*/

Please follow ScalaDoc standard practice: https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html

More importantly, I don't actually intuitively know what subtracting one BitPat from another means. I don't think relying on regular mathematical intuition here is right, I think this doc needs to explain with examples what this means.

[jackkoenig]

This was marked resolved but not addressed. What does it mean to subtract two BitPats? Even an example in the ScalaDoc would go a long way.

[jackkoenig]

A couple of minor comments and unresolved old comments. This looks awesome, let's get this in.

I am a little nervous about BitSet just being a new API. Could/Should we make it experimental?

[sequencer]

Could/Should we make it experimental?

I think we should, however a non-experimental API BitPat depends on an experimental seems strange. So I experimental the BitSet object, and seal the BitSet API.

then

import chisel3.util.experimental.BitSet
import chisel3.util.BitSet

seems to be crazy :(

I'm not sure should I experimental both class and object now...

leave this decision to @jackkoenig ;p

288181b is non-experimental BitSet
56f61e6 is experimental object BitSet
34f313e is experimental class BitSet and object BitSet

[jackkoenig]

I'd rather we could find a way to make the new stuff experimental, but as far as I can tell it's very awkward no matter what we do. So I'm fine with BitSet being non-experimental. I remain a bit nervous about there being issues with it that get hard to fix as a normal API, so please use this a bunch before we release 3.5.0 to find any issues. Regardless, let's keep both it and BitPat sealed.

Ignore this, see below.

[jackkoenig]

I'd rather we could find a way to make the new stuff experimental, but as far as I can tell it's very awkward no matter what we do. So I'm fine with BitSet being non-experimental. I remain a bit nervous about there being issues with it that get hard to fix as a normal API, so please use this a bunch before we release 3.5.0 to find any issues. Regardless, let's keep both it and BitPat sealed.

What I mean by this, is that even if we make BitSet experimental, some of the new BitPat methods are defined on non-experimental BitPat and thus we're sort of stuck with. They also return non-experimental BitSet.

That being said, I think an imperfect amount of experimental is still better than none. So I'm now thinking that we should keep object and class BitSet as experimental. It's slightly awkward, but it at least calls out that these new APIs are new. We can see how they do in 3.5 and make them regular chisel3.util in 3.6 (and let's make that be faster than 3.4 -> 3.5 was 🙂 )

[jackkoenig]

LGTM! Just need to fix the CI failure.

[sequencer]

I think I need to add additional tests on this PR, I will work on this today.