Skip to content

Conversation

@carlosedp
Copy link
Contributor

@carlosedp carlosedp commented Nov 25, 2021

This PR adds a One-Hot encoded ChiselEnum where the elements have 1H instead of sequential IDs:

// It simplifies:
object InstructionType extends ChiselEnum {
  val INST_I = Value((1 << 0).U)
  val INST_S = Value((1 << 1).U)
  val INST_B = Value((1 << 2).U)
  val INST_U = Value((1 << 3).U)
  val INST_J = Value((1 << 4).U)
  val INST_Z = Value((1 << 5).U)
  val INST_R = Value((1 << 6).U)
  val IN_ERR = Value((1 << 7).U)
}

// into:

object InstructionType extends ChiselEnum1H {
	val INST_I, INST_S, INST_U, INST_B, INST_J, INST_Z, INST_R, IN_ERR = Value
}

// Allowing the use in Mux1H for example as:
  val selectorVal = Wire(UInt(InstructionType.getWidth.W))
  selectorVal := InstructionType.INST_I.asUInt
  val hotValue = chisel3.util.Mux1H(
    selectorVal,
    Seq(
      2.U,
      4.U,
      8.U,
      11.U,
      15.U
    )
  )

The PR is for discussion and in case of greenlit, I'll add test cases for it.

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

  • documentation
  • new feature/API

API Impact

The PR creates a ChiselEnum where elements have One-Hot IDs.

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

This PR adds a One-Hot encoded ChiselEnum where the elements have One-Hot IDs instead of sequential. This change pairs well with Mux1H.

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?

@carlosedp carlosedp changed the title Create OneHotChiselEnum where elements have one-hot encoding IDs Create ChiselEnum1H where elements have one-hot encoding IDs Nov 25, 2021
Comment on lines 31 to 34
* 2.U,
* 4.U,
* 8.U,
* 11.U,
Copy link
Member

Choose a reason for hiding this comment

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

I think we need add a sugar for Mux1H to consume a Enum type and return a Enum type.
These magic num is strange, while seems not correct: 11.U -> 16.U

Copy link
Member

Choose a reason for hiding this comment

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

This can be done in future PR(just comment for recording)

Copy link
Member

Choose a reason for hiding this comment

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

I think we need add a sugar for Mux1H to consume a Enum type and return a Enum type.
These magic num is strange, while seems not correct: 11.U -> 16.U

@sequencer
Copy link
Member

My further question to this is: how to design the encoding API? 1H API implicitly contains an assertion: PopCount === 1.U.
And how do we resolve this general encoding(ECC, 8b/10b, Gray Code, Hamming Code, etc...) issue?
Should we directly add corresponding assertion to this, which makes verification easier?
Or should we annotate the these encoding signal with an Annotation? and optionally add a Transform to these signals?
And how to propagate the encoding type? In bind function or propagate in FIRRTL transform?

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

Generally LGTM.
I strongly depend on EnumOH, it is really good for code simplification.
The only thing we should do is to private the non-user api.
in the future, we should figure out a better api to integrate EnumOH to MuxOH.

Comment on lines 31 to 34
* 2.U,
* 4.U,
* 8.U,
* 11.U,
Copy link
Member

Choose a reason for hiding this comment

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

This can be done in future PR(just comment for recording)

@carlosedp carlosedp force-pushed the onehotenum branch 2 times, most recently from dbb8601 to 1dc1c2d Compare December 13, 2021 22:57
@carlosedp carlosedp requested a review from sequencer December 13, 2021 23:02
@sequencer
Copy link
Member

@carlosedp would you mind adding some test to demonstrate the usage?

@carlosedp
Copy link
Contributor Author

Ping... news on this?

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

a little nitpick.

Comment on lines 31 to 34
* 2.U,
* 4.U,
* 8.U,
* 11.U,
Copy link
Member

Choose a reason for hiding this comment

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

I think we need add a sugar for Mux1H to consume a Enum type and return a Enum type.
These magic num is strange, while seems not correct: 11.U -> 16.U

@carlosedp
Copy link
Contributor Author

In the sample below, I really don't like the line to set the selector width...

   object selector extends chisel3.experimental.ChiselEnum1H {
     val s0, s1, s2, s3 = Value
   }
   val selectorVal = Wire(UInt(selector.getWidth.W)) // <--- don't like this
   selectorVal := selector.s1.asUInt
   val hotValue = chisel3.util.Mux1H(selectorVal.asUInt,
      Seq(
       2.U,
       4.U,
       8.U,
       16.U,
     ))

Any ideas on improving this?

@ekiwi
Copy link
Contributor

ekiwi commented Dec 28, 2021

Any ideas on improving this?

Some thoughts:

  1. It seems to me like Mux1H should be changed to accept ChiselEnum1H instead of a UInt directly.
  2. You should be able to directly use your selector type as a type and thus write something like val selectorVal = Wire(selector).
  3. It might also make sense to have a toIndex function for ChiselEnum1H such that e.g. assert(selector.INST_S.toIndex === 1.U)

@carlosedp
Copy link
Contributor Author

For 1, I could send another PR... ok?
On 2, you mean the way it's already implemented I could use like this?
Working on 3.

@ekiwi
Copy link
Contributor

ekiwi commented Jan 5, 2022

For 1, I could send another PR... ok?

I would prefer to see everything work together in a single PR.

On 2, you mean the way it's already implemented I could use like this?

Yes. Can you try to change your example and see if it works?

Working on 3.

Awesome

@carlosedp
Copy link
Contributor Author

There it go! Have Mux1H accept ChiselEnum1H and added the toIndex function.
Also example is pretty simple now:

  object selector extends chisel3.experimental.ChiselEnum1H {
    val s0, s1, s2, s3 = Value
  }
  val selectorVal = selector.s1
  val hotValue = chisel3.util.Mux1H(selectorVal,
      Seq(
      2.U,
      4.U,
      8.U,
      16.U,
    ))
  // hotValue is 4.U

@carlosedp carlosedp force-pushed the onehotenum branch 2 times, most recently from b18724b to 515a2bf Compare January 5, 2022 22:08
@carlosedp carlosedp mentioned this pull request Jan 10, 2022
14 tasks
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
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.

4 participants