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

First crack at updating the readme #1106

Merged
merged 12 commits into from
Jun 19, 2019
Merged

First crack at updating the readme #1106

merged 12 commits into from
Jun 19, 2019

Conversation

chick
Copy link
Contributor

@chick chick commented Jun 4, 2019

Type: documentation

Goals
Include up front example
Simplify
Get users to things quicker
Move complicated details to wiki

No functional change

may require some updates to wiki

Release Notes
Update the readme to provide simpler and quicker access to primary resources

Goals
Include up front example
Simplify
Get users to things quicker
Move complicated details to wiki.
@chick chick requested a review from a team as a code owner June 4, 2019 18:02
@chick chick self-assigned this Jun 4, 2019
intellij and github don't use same render for .md files
@chick
Copy link
Contributor Author

chick commented Jun 4, 2019

Please bike shed, I don't want to go to far with this re-design without some feedback.

Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

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

One minor typo/grammatical error.
There's a lot of Scala magic in the FIR - I'm afraid it may be too daunting for beginners.

README.md Outdated
# What does Chisel code look like?

Chisel is a [Scala](https://www.scala-lang.org/) embedded DSL, it takes advantage of object oriented and functional techniques to create complex
circuit generators with a minimum of code. Parameterized generators with strong type and error checking builds re-usable stuff fast.
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameterized generators with strong type and error checking build re-usable stuff fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to add a different example, I looked at a number of things in the bootcamp and nothing leapt out at me as having sufficient complexity (I think we need more than a hello world here, others may disagree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo fixed

Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

Lots of comments. I also agree that the example is too complex, but I think that can be solved by a combination of better style and reducing the complexity to a basic / core FIR generator instead of a do-it-all monster. I think FIR is nice because it's something that I'd expect most engineers to understand, while also showcasing something that should be much easier in Chisel compared to Verilog.

README.md Outdated
@@ -6,109 +6,87 @@
[![CircleCI](https://circleci.com/gh/freechipsproject/chisel3/tree/master.svg?style=shield)](https://circleci.com/gh/freechipsproject/chisel3/tree/master)
[![GitHub tag (latest SemVer)](https://img.shields.io/github/tag/freechipsproject/chisel3.svg?label=release)](https://github.com/freechipsproject/chisel3/releases/latest)

Chisel is a new hardware construction language to support advanced hardware design and circuit generation.
Chisel is a powerful hardware construction language to support advanced hardware design and circuit generation.
The latest iteration of [Chisel](https://chisel.eecs.berkeley.edu/) is Chisel3,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop iterations of Chisel, since it's basically chisel3 or nothing. Iteration also implies that we're going to make sweeping changes in the near future (which implies backwards compatibility issues), which we don't.

README.md Outdated

# What does Chisel code look like?

Chisel is a [Scala](https://www.scala-lang.org/) embedded DSL, it takes advantage of object oriented and functional techniques to create complex
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain DSL. To hardware engineers who aren't PL experts. What is it, why does it matter, and how does it affect them from a user-facing perspective?

I'd rather say something along the lines of using Scala to write a program that constructs circuits (though we should also emphasize the HDL-like syntax). Maybe refer to it in familiar terms, like Perl minus the dumpster fire.

README.md Outdated
Chisel is a [Scala](https://www.scala-lang.org/) embedded DSL, it takes advantage of object oriented and functional techniques to create complex
circuit generators with a minimum of code. Parameterized generators with strong type and error checking builds re-usable stuff fast.
```scala
// This FIR filter has parameterized window length, IO bitwidth, and windowing function
Copy link
Contributor

Choose a reason for hiding this comment

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

Intro text should go in the paragraph above. Don't necessarily expect people to read the comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also consider adding a diagram, either as a .svg or as ASCII art, of what the generated hardware looks like.

README.md Outdated
circuit generators with a minimum of code. Parameterized generators with strong type and error checking builds re-usable stuff fast.
```scala
// This FIR filter has parameterized window length, IO bitwidth, and windowing function
class MyFir(length: Int, bitwidth: Int, window: (Int, Int) => Seq[Int]) extends Module {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also second making this example less complicated. A minimalistic FIR should be fine for illustration purposes, it doesn't necessarily have to be how a DSP expert might write this.

Copy link
Contributor

Choose a reason for hiding this comment

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

More importantly, if you want to showcase the power of abstraction, consider starting with how you use the FIR generator, then showing the implementation. Because most users will probably import and call an off-the-shelf FIR generator library.

README.md Outdated
class MyFir(length: Int, bitwidth: Int, window: (Int, Int) => Seq[Int]) extends Module {
val io = IO(new Bundle {
val in = Input(UInt(bitwidth.W))
val out = Output(UInt((bitwidth*2+length-1).W)) // expect bit growth, conservative but lazy
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider just making it bitwidth and placing the burden on the user to size it correctly?

README.md Outdated

[Download and install sbt for Windows](https://www.scala-sbt.org/download.html).
For more a more complex template that includes rocket-chip and more, visit the [rebar repository](https://github.com/ucb-bar/project-template).
Copy link
Contributor

Choose a reason for hiding this comment

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

rocket-chip and rebar don't mean anything to someone who would be getting something out of reading this document. If you feel it's still important to list those, consider having a short description, that answers both the what and why.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. more complex template which includes the rocket-chip RISC-V processor generator etc

README.md Outdated

[Download and install sbt for Windows](https://www.scala-sbt.org/download.html).
For more a more complex template that includes rocket-chip and more, visit the [rebar repository](https://github.com/ucb-bar/project-template).
To learn more about installing Chisel3 visit [The wiki installation page](Installation)
Copy link
Contributor

Choose a reason for hiding this comment

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

The link is dead.

README.md Outdated
```
yaourt -S firrtl-git verilator sbt
```
When you are ready to build and test your own circuits. Chisel3 uses the Sonatype/Nexus/Maven package management systems to seamlessly deliver the environment and extension
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is where you should link the installation instructions.

Also, if you're encouraging people to use the template, you don't need to go much into Sonatype/Nexus/Maven, since that's all abstracted away. But you might consider phrasing it like:

When you are ready to build and test your own circuits, check out the [install instructions] for your platform. We recommend starting your project from the [chisel-template], though you can also [manually set up your project] to use the managed dependencies published on Sonatype/Nexus/Maven.

README.md Outdated

### Mac OS X
## [Chisel3 Cheat Sheet](https://chisel.eecs.berkeley.edu/doc/chisel-cheatsheet3.pdf)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue the cheatsheet should go first. It's more curated (though perhaps somewhat out of date) compared to the API docs, which I think highly varies in quality.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually say wiki -> cheatsheet -> API

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends on the degree of completeness and curation. I don't know much about how complete the wiki is, but the cheatsheet is intended to be a fairly complete and uniformly polished (though brief) overview of the APIs, and I think there's serious value in that compared to a more detailed but also sparse documentation. In any case, they're both complementary, and ordering might be less important than a short blurb about what each resource is intended to be used for.

README.md Outdated
The latest iteration of [Chisel](https://chisel.eecs.berkeley.edu/) is Chisel3,
which uses Firrtl as an intermediate hardware representation language.
which uses Firrtl as an intermediate hardware representation language. Chisel promotes re-use through parameterization and
Copy link
Contributor

Choose a reason for hiding this comment

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

For users, FIRRTL is going to be an implementation detail, though I see value in keeping the link to promote FIRRTL. But do so in a way where it doesn't make people more confused - it shouldn't give the impression that one needs to learn another language to use Chisel.

Verilog should also be mentioned somewhere, because I could see one question being, "all my tools are in Verilog, can I use this for actually (building chips | pushing stuff to FPGA) or is this just a toy design language"

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how exactly to do this - making it clear that Chisel can generate Verilog but it's not just a Verilog templating engine is useful (e.g. maybe emphasing the use of OOP, FIRRTL, etc)

Compact the fir-filter code a bit
@seldridge
Copy link
Member

seldridge commented Jun 6, 2019

For the FIR example, I think we should do several things:

  • Make this as terse as possible. Clash uses an FIR filter on their website and they keep it nice and compact.
  • I agree with @ducky64 in that the filter should be parameterized by coefficients and not by a window function.
  • The length parameter should be implicit based on the number of coefficients
  • We should not use an anonymous/structurally typed bundle for the IO
  • (Maybe) The FIR filter should be parameterized in terms of data type and not locked to UInt
  • We should include examples of using the filter, e.g., for different windows/coefficients
  • We should show the output of this in Diagrammer as opposed to with some custom SVG

I'm still golfing this down, but the following would be a start. (Not tested, though!)

class FIRFilterIO(length: Int, bitWidth: Int) extends Bundle {
  val in = Input(UInt(bitWidth.W))
  val out = Output(UInt((bitWidth * 2 + length - 1).W))
}

class FIRFilter(bitWidth: Int, coefficients: Seq[Int]) extends Module {
  val io = IO(new FIRFilterIO(coefficients.size, bitWidth))

  val z = Seq.fill(coefficients.size - 1){ Reg(UInt(bitWidth.W)) }
    .scan(io.in){ case (r, l) => l := r; l }
    .drop(1)

  io.out := coefficients
    .zip(io.in +: z)
    .map{ case (b, x) => b.U * x }
    .reduce(_ +& _)
}

@chick
Copy link
Contributor Author

chick commented Jun 6, 2019

@ducky64 @seldridge Ok trying again. This down to a single assignment. I had to make the coefficients UInts to avoid conversion. Adding a generator is tricky here without bringing in DspTools to manage the type management. Unless I'm missing something making signature something like
FIR[T <: Data](gen: T, coeff: Seq[T]) is hard to make the code work

class FIR(bitWidth: Int, coeffs: Seq[UInt]) extends MultiIOModule {
  val (in, out) = (IO(Input(UInt(bitWidth.W))), IO(Output(UInt(bitWidth.W))))

  out := coeffs
          .zip(in +: coeffs.tail.scan(in) { case (a: Data, _) => RegNext(a) })
          .map { case (c, r) => c * r }
          .reduce(_ +% _)
}

@edwardcwang
Copy link
Contributor

One question on the FIR example - what's our intended audience? Software functional programmers looking to learn some hardware, or digital designers looking to use/adopt Chisel? Depending on the audience the example could be attractive or repulsive. Maybe consider two examples for different audiences?

@ducky64
Copy link
Contributor

ducky64 commented Jun 7, 2019

I'd argue that we should try to make the code should be understandable by the average software engineer (let's say, Java, C, C++ familiarity, experience with functional programming at the level of Python) and intuitively readable by the average hardware engineer who has some concept of what map and reduce does (ideally, this wouldn't be required, but I don't think we can make a powerful example without those fundamental constructs; compromise would be for the comments to patch across knowledge gaps).

I think code golf should be an explicit anti-goal, and breaking things up into multiple assign statements with comments as necessary can vastly increase readability. When the competition is the dumpster fire that is Perl generating (System)Verilog, I think achieving less-lines-of-code is already trivial, so there's no need to compromise on readability.

I also think we should try to avoid uncommon (in terms of the average audience - think Java, C, C++, Python, Verilog) syntax where possible in favor of more explicit constructs. Case statements should probably be out, and _, +:, and +% are debatable since symbol soup can be confusing. Deeply nested parentheses, like in the zip line, might also be questionable.

@seldridge
Copy link
Member

seldridge commented Jun 7, 2019

This is great feedback... I think there are competing Chisel features that we need to balance including:

  1. Chisel is the same as Verilog (with restrictions that prevent user error)
  2. Chisel is more "powerful" than Verilog/VHDL
  3. Chisel provides a hardware compiler framework

The current example shows (2) only.

(1) is related to @edwardcwang's comment. Perhaps two examples, one that is (non-parameterized) Verilog translation and one that pushes the language. (1) is also tricky as there are advanced Chisel features (withClockAndReset, async reset, etc.) that provide Verilog parity, but likely are out of scope of an example for (1).

(2) could be shown by re-implementing (1) as parameterized with functional programming features.

(3) is not shown, but could be, e.g., BoringUtils, Verilog attribute injection, flatten/inlining. These, however, may require showing output Verilog/FIRRTL.

@ducky64: point taken; the golfing may be unnecessary here. This is more motivated by how Haskell is commonly presented as being "more powerful" than other languages, e.g., you can tightly represent this complex imperative construct with a series of functions. For reference, they use this example on their landing page. This is more an aspirational example of "Woah, I'd like to learn how to do that." One data point...

primes = filterPrime [2..]
  where filterPrime (p:xs) =
          p : filterPrime [x | x <- xs, x `mod` p /= 0]

One more idea to consider, pushing towards showing (1) and (2):

class FIRIO[A <: Data with Num[A]](dataType: A) extends Bundle {
  val in = Input(dataType.cloneType)
  val out = Output(dataType.cloneType)
}

/** A 3-coefficient FIR filter  */
class FIRRectangular3(bitWidth: Int) extends Module {
  val io = IO(new FIRIO(UInt(bitWidth.W)))

  val b0 = 1.U
  val b1 = 1.U
  val b2 = 1.U

  val z0 = RegNext(io.in)
  val z1 = RegNext(z0)

  val p0 = io.in * b0
  val p1 = z0 * b1
  val p2 = z1 * b2

  io.out := p0 + p1 + p2
}

/** A generator of FIR filters of arbitrary coefficients and underlying data type (UInt, SInt, FixedPoint) */
class FIRGenerator[A <: Data with Num[A]](dataType: A, coeffs: Seq[A]) extends Module {
  val io = IO(new FIRIO(dataType))

  def dotProduct(a: Seq[A], b: Seq[A]): A =
    a.zip(b)                     // (a0, a1, a2), (b0, b1, b2)     => ((a0, b0), (a1, b1), (a2, b2))
      .map{ case (x, y) => x*y } // ((a0, b0), (a1, b1), (a2, b2)) => ((a0*b0), (a1*b1), (a2*b2))
      .reduce(_ + _)             // ((a0*b0), (a1*b1), (a2*b2))    => a0*b0 + a1*b1 + a2*b2

  /** Construct registers that will hold time-delayed versions of the input */
  val z: Seq[A] = Seq.fill(coeffs.size - 1){ Reg(dataType.cloneType) }

  /** Construct a shift register from the input and time delay registers */
  z.foldLeft(io.in){ case (right, left) => left := right; left }

  /** Compute a dot product of the inputs/time delays with the coefficients */
  io.out := dotProduct(io.in +: z, coeffs)
}

class GeneratedFIRRectangular3 extends FIRGenerator(UInt(8.W), Seq(1.U, 1.U, 1.U))

class FIRFilterSpec extends ChiselFlatSpec {

  "foo"  should "foo" in {
    println(generateFirrtl(new FIRRectangular3(8)))
  }

  "bar" should "bar" in {
    println(generateFirrtl(new GeneratedFIRRectangular3))
  }

}

@chick
Copy link
Contributor Author

chick commented Jun 7, 2019

All this is great stuff for consideration but I think the immediate decision should be whether what I have done is better than what is currently there. I would argue that it is. The current page rambles on about ugly installation stuff that is not relevant. The new version shows some code then gets you to the bootcamp and template fast. I believe solving this was what I was tasked with doing. If someone wants to inject some other piece of code that's fine with me, but this PR should be acted on soon or dropped for further debate and consideration. I vote for going simple and soon to be followed up by more later.

@ducky64
Copy link
Contributor

ducky64 commented Jun 7, 2019

@chick I think there's still some work to be done:

  • I think we need to have a good idea of who the target audience is for the overview. Is it PL academics? Is it software engineers looking to play wtih FPGAs? Or is it hardware engineers who wouldn't have a formal PL background? Because as written right now, a lot of terms are thrown around which might be correct but not meaningful to all members of these audiences. Example: hardware construction language, domain-specific language (totally an implementation detail), strong type checking.
  • Also, what's the message? The overview currently seems to be a mishmash of various facts about Chisel, but I'd imagine what a potential user needs answered is, "why should I use this", and more implicitly, "how does this compare against what I'm using right now".
  • Basically, the overview is probably going to be the first thing people read (if they find this repo first, as opposed to the website - though we might consider unifying the blubs), so you want to make a good impression there. You really only get one first impression.
  • I think it's worth discussing this in realtime at the next dev meeting.
  • One benchmark for a good intro: find a few 151 students, show them the overview text, and ask them how they interpreted it, and whether they would try Chisel. And keep in mind that this is even a bit of a low bar for intro text: 151 students would have taken the 61a/b/c series and have a reasonable programming background.
  • https://www.chisel-lang.org/ gives a cert error. Because the cert is issued to research.eecs.berkeley.edu...
  • Lots of copy-editing, the little things that make the documentation look more professional and clean instead of an afterthought / completely slapdash. Unless you intend this to be a rough draft, in which case, sure. Example: inappropriate use of headers to make text larger, which puts a header underline and appears as a section that doesn't yet have content. Caps in middle of sentence ("visit The wiki installation page"), broken sentences ("When you are ready to build and test your own circuits. Chisel3 uses"). You know, the kind of things that would say to users "hey, we really care about having users and are doing our best to support them", rather than "we spent the minimum amount of time putting together this minimum-viable-readme".
  • I'd also argue the front-page example is pretty important: it's the one place where you're showing rather than telling. And one would hope what is shown reinforces what is told.

@seldridge I think in designing the example, there's kind of an overarching question of whether we want to be more like Python (powerful but still mass-adoption friendly) or Haskell (super efficient for power users, but steep learning curve). It appears much more programmers use Python than Haskell, though we can only conjecture at the causality (why).

  • Consider putting yourself in the shoes of someone in your target audience and showing them the code snippet.
    • For the Haskell-style folks, the response might reasonably be "I'll spend some quality time reading through and and trying to comprehend this example because it's so concise."
    • For the Python-style folks, the response might reasonably be "wtf is this ****, it makes no sense at all. Wow, this language seems like it could have quite the nasty learning curve."
  • I'd also recommend showing this to hardware engineers who haven't used Chisel before and asking for feedback.
  • Remember that there is always a third option: you can place the easily understandable example in the readme, but link to more advanced examples. The message would be kind of "this example shows that Chisel is miles ahead of the dumpster fire that is Perl generating Verilog, but if you're into playing code golf rather than practical software engineering we've [got you covered too]"

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
seldridge and others added 3 commits June 7, 2019 16:26
README.md Outdated Show resolved Hide resolved
@ducky64 ducky64 mentioned this pull request Jun 14, 2019
@ucbjrl ucbjrl merged commit c6376e7 into master Jun 19, 2019
@ucbjrl ucbjrl deleted the readme-update-1 branch June 19, 2019 17:08
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
* Add Test for AddDefaults phase

* Refactor AddDefaultsSpec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants