Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Add Cookbook section on aliased Bundle fields #2444

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

jackkoenig
Copy link
Contributor

See post mdoc rendered markdown here.

Contributor Checklist

  • [NA] 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?
  • [NA] Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • documentation

API Impact

No impact

Backend Code Generation Impact

No 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?

@jackkoenig jackkoenig added this to the 3.5.x milestone Mar 11, 2022
@jackkoenig jackkoenig requested a review from mwachs5 March 11, 2022 20:43

#### 4. Call `.cloneType` directly

You can also just call `.cloneType` on your `gen` argument directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I have always been wondering about: Why don't we allow calling chiselTypeOf in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Chisel2, the distinction between types and values was super blurry so users often ended up just peppering .cloneType everywhere to make sure things worked correctly. The idea in Chisel3 was to have a stronger distinction here--binding operators only take types, and chiselTypeOf only converts from hardware values back to types.

That being said, my use of .cloneType here is back to the Chisel2 style and is admittedly just avoiding this issue.

Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

At a high level, should we rename this article to "how to properly use gen args in your bundles". Also, should we pick one recipe to recommend? As this is a cookbook not exhaustive reference on all the options?

But thanks for writing this, it actually helped me understand why I never quite knew which pattern to use... they are all equivalent!

docs/src/cookbooks/cookbook.md Outdated Show resolved Hide resolved
@jackkoenig
Copy link
Contributor Author

jackkoenig commented Mar 14, 2022

At a high level, should we rename this article to "how to properly use gen args in your bundles". Also, should we pick one recipe to recommend? As this is a cookbook not exhaustive reference on all the options?

Probably, but I was trying to avoid taking a position at the moment. I think we should decide on a style to recommend, but it's tricky because the style that leads to this problem is really really common. I think I'd like to merge this example as is since it is useful, but we should think about what style we want to recommend and then go with it.

To elaborate a little bit on why I'm avoiding taking a position, here's a little discussion:


Field

Note that this is a long running option and there was an old proposal to add Field(...) as a new undirectioned operator analogous to Output that you could use in this case. The rub there is that to be consistent it would be required on all members of the Bundle, eg. if you had something like

class MyBundle[T <: Data](gen: T) extends Bundle {
  val foo = UInt(8.W)
  val bar = UInt(32.W)
  val fizz = gen
  val buzz = Vec(2, UInt(8.W))
}

If you want to add another field to MyBundle that uses gen and we had Field, the new Bundle would be:

class MyBundle2[T <: Data](gen: T) extends Bundle {
  val foo = Field(UInt(8.W))
  val bar = Field(UInt(32.W))
  val fizz = Field(gen)
  val buzz = Field(Vec(2, UInt(8.W)))
  val other = Field(gen)
}

People understandably find this a bit annoying. Also note that this same problem applies to the current legal approach of just using Output.

Thunkification () => T

So on the flip side, say we recommend () => T or => T. These probably make the most sense but have their own issues being infectious from bottom up. Say we have MyBundle from above and we use it in some larger design:

// Some datastructure of parameters to my generator
case class GeneratorParams[T <: Data](x: Int, y: Int, gen: T)

class MyModule[T <: Data](params: GeneratorParams) extends Module {
 ...
  val foo = IO(new MyBundle(params.gen))

Now the problem, is when someone updates MyBundle to MyBundle3:

class MyBundle[T <: Data](gen: () => T) extends Bundle {
  val foo = UInt(8.W)
  val bar = UInt(32.W)
  val fizz = gen()
  val buzz = Vec(2, UInt(8.W))
  val other = gen()
}

The will likely naively update all calls like so:

class MyModule[T <: Data](params: GeneratorParams) extends Module {
 ...
  val foo = IO(new MyBundle(() => params.gen))
                         // ^ Problem

This will give the exact same aliasing error. The problem is that the "thunkification" of gen must go all the way up to the top.

So what's the point of this stream of consciousness? It's not clear what we should recommend so at the moment I'm just trying to be useful without resolving the more difficult discussion 🙂

@jackkoenig jackkoenig merged commit d4dd211 into master Mar 15, 2022
@jackkoenig jackkoenig deleted the aliased-field-cookbook branch March 15, 2022 00:23
mergify bot pushed a commit that referenced this pull request Mar 15, 2022
@mergify mergify bot added the Backported This PR has been backported label Mar 15, 2022
mergify bot added a commit that referenced this pull request Mar 15, 2022
(cherry picked from commit d4dd211)

Co-authored-by: Jack Koenig <koenig@sifive.com>
jackkoenig added a commit that referenced this pull request Feb 28, 2023
* Add renamemap.MutableRenameMap which includes these methods without
  deprecation
* Deprecate Stringly typed RenameMap APIs which were accidentally
  undeprecated a while ago

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants