Verilog

[schoeberl]

Contributor Checklist

This is just a start for a discussion to resolve #1715. Not intended to merge now, before we finished the discussion.

This is a quick fix to add Verilog emission to a Chisel3 object, instead of ChiselStage or new ChiselStage. However, I would prefer to have this functionality on Module, like this:

(new Hello()).emitVerilog

where Hello() is a Chisel Module.

[seldridge]

I actually really like your API of: (new Hello()).emitVerilog. I have no idea how to realize this given the constraints of needing to be in a builder environment.

[sequencer]

I think API like (new Hello()).emitVerilog sounds really good!(something like driver, but much cleaner)
Here are some of my thinking:

1. I doubt if it is necessary to add an object Chisel for this API, or maybe we can alias a ChiselStage object under chisel3 package?
2. Can we unify APIs from object and class of ChiselStage? I think they are kind of duplicated, or we can add more APIs to the object ChiselStage, so that, no more need to new one.
3. I really like an API like (new Hello()).emitVerilog(annos: AnnotationSeq = Nil, args: Array[String] = Nil) I think it can be an implicit class(maybe chisel3.experimental.utils), which needs to import explicitly to add this API. But as @seldridge said, new Hello() should be guarded under the Builder. So basically, we need an call-by-reference here so I wonder if it is possible to call-by-reference under an implicit function, I will have a try:)

[sequencer]

Haha, it works:)

[seldridge]

Haha, it works:)

Call-by-name implicit class? What is this black magic?

🤯 🤯 🤯

[sequencer]

Call-by-name implicit class? What is this black magic?

LOL, really strange API, but it works, just a prototype, we can discuss this API on next dev-meeting, interesting, but maybe a liitle brittle.

[schoeberl]

Great! Looking forward to this lean API to generate Verilog.

I actually did not understand in the first place that I was not able to access chisel3.stage members within the chisel3 located code when it was in the core folder.

[schoeberl]

This is now the version using the trick shown us by @sequencer. However, I do not really like that implementation: (1) too much magic in the implicit class thing and (2) we need an additional import (e.g., chisel3.verilog._ package name is up for discussion. Maybe implicit?) to make this implicit conversion possible.

This is all because I cannot call into chisel3.stage.foo() from RawModule.

However, the API looks now the way I like it, just the import is a bit annoying. If we do not change the way core Chisel cannot access stage stuff, looks like we do it this way.

If we agree on this, I can finish this with cleanup, maybe more tests, and depricating the ChiselStage versions.

[sequencer]

(1) too much magic in the implicit class thing

IMHO, I think it's good to use a implict class to do such job(only explicitly import with a package won't introduce too much confusing). the only black magic are implicit class and call-by-reference, personally think it's OK.

(2) we need an additional import (e.g., chisel3.verilog._ package name is up for discussion. Maybe implicit?) to make this implicit conversion possible.

I think it is good, only import when it is explicitly needed, think about this case: if user defined a conflict API to this?:

import chisel3._
class A extends Module {
  def emitVerilog:String = "something strange, but valid" 
}
object Main {
  // I think this is a good reason not put this API under chisel3 package
  (new A).emitVerilog
}

so my person idea is put these implicit conversion under a place where not exist before, something like chisel3.stage.implicit, so that it is not possible to break any current user codes.

[schoeberl]

Sorry, but I disagree ;-)

(1) too much magic in the implicit class thing IMHO, I think it's good to use a implict class to do such job(only explicitly import with a package won't introduce too much confusing). the only black magic are implicit class and call-by-reference, personally think it's OK.

This implicit trick is really just needed because the implementation of Chisel is split (artificially or probably historically?) into two projects. If it would live in just one project, we could simply implement the method on RawModule. No funny tricks needed.

(2) we need an additional import (e.g., chisel3.verilog._ package name is up for discussion. Maybe implicit?) to make this implicit conversion possible. I think it is good, only import when it is explicitly needed, think about this case: if user defined a conflict API to this?:

I disagree here as well. Generating Verilog is fundamental to Chisel, not a special case. Why confuse users with additional packages here. I guess my role here is to be the advocate of newcomers to Chisel and digital design, such as my students ;-) Let’s keep the standard usage of Chisel as simple as possible. Almost unrelated: I might even question the benefit of a package chisel3.util. The switch statement is fundamental for HW design, why make it optional?

so my person idea is put these implicit conversion under a place where not exist before, something like chisel3.stage.implicit, so that it is not possible to break any current user codes.

However, I guess I can live with an import of something like chisel3.verilog._. For sure not implicit as package name. This has no meaning for a Chisel user/HW developer. This is an internal implementation detail. just my 2c, Martin

[sequencer]

This implicit trick is really just needed because the implementation of Chisel is split (artificially or probably historically?) into two projects. If it would live in just one project, we could simply implement the method on RawModule. No funny tricks needed.

Not really. The implicit convention is making sure Module is call-by-reference, since. Module must be build from a Builder context.
I personally think splitting project is reasonable. core is DSL frontend library, while chisel3 defines how to use that DSL libray. I think this dependency is tidy, but this should be a question to Jack. I’m not so 100% for sure here.

Almost unrelated: I might even question the benefit of a package chisel3.util. The switch statement is fundamental for HW design, why make it optional?

import chisel3._

means importing from the core library, while

import chisel3.util._

means a standard library which depends on core library, using those meta API to construct other circuit.
I have to admit switch case is a good API, however, while FIRRTL doesn’t have the case semantic, so this is just a wrapper to when block, this is the reason why it lives in util package.

However, I guess I can live with an import of something like chisel3.verilog._. For sure not implicit as package name. This has no meaning for a Chisel user/HW developer. This is an internal implementation detail.

I have no preference to package name, we can discuss API and package name in the dev-meeting. But at least, the implicit conversion should be put at a package where never belong to chisel3. Since after user bump version, they may accidentally import the implicit conversion, I personally think this is a case which must be avoid.

[schoeberl]

If we are not planning a major change by moving stage.* to core, I guess this small addition is ready for review. Shall we deprecate the two versions of emitVerilog in ChiselStage?

[jackkoenig]

My main concern with this idea is that it is a bit weird that:

// This works
(new PlusOne()).getVerilogString

// But this doesn't
val mod = new PlusOne()
mod.getVerilogString
// java.lang.IllegalArgumentException: requirement failed: must be inside Builder context
//	at scala.Predef$.require(Predef.scala:281)
//	at chisel3.internal.Builder$.dynamicContext(Builder.scala:339)
//	at chisel3.internal.Builder$.readyForModuleConstr(Builder.scala:499)
//     ...

I know why of course, but I don't expect a new user to. If we're providing a friendlier, more obvious API for users I think we should be careful here, and this feels a bit haphazard to me.

This doesn't really compose with running it inside of elaboration, it's a double elaboration which may surprise people writing something like:

// Inside a module
val inst = Module(new MyModule)
inst.getVerilogString  // Actually I don't know what happens here, what happens?

My issue here isn't with the idea, I like it a lot. I'm just really not sure that a method-like API is the right one when it's fundamental to the architecture of Chisel3 that you need to construct the module within a Builder context. We can maybe make this work with thoughtful error reporting for when people do it wrong, but something about the method-style invocation worries me about user-experience since it will often not work as one might expect (ie. erroring for non-obvious reasons).

In my opinion, we should do something more similar to @schoeberl's original commits. I'm even fine with just putting them directly in chisel3 if we're really trying to make this as simple as possible. I'm open to being convinced otherwise though.

[sequencer]

// Inside a module
val inst = Module(new MyModule)
inst.getVerilogString  // Actually I don't know what happens here, what happens?

This is easy to guard if implicit class can access Builder context.

// This works
(new PlusOne()).getVerilogString

// But this doesn't
val mod = new PlusOne()
mod.getVerilogString
// java.lang.IllegalArgumentException: requirement failed: must be inside Builder context
//	at scala.Predef$.require(Predef.scala:281)
//	at chisel3.internal.Builder$.dynamicContext(Builder.scala:339)
//	at chisel3.internal.Builder$.readyForModuleConstr(Builder.scala:499)
//     ...

Actually, this error message will be

// But this doesn't
val mod = new PlusOne()
// java.lang.IllegalArgumentException: requirement failed: must be inside Builder context
//	at scala.Predef$.require(Predef.scala:281)
//	at chisel3.internal.Builder$.dynamicContext(Builder.scala:339)
//	at chisel3.internal.Builder$.readyForModuleConstr(Builder.scala:499)
//     ...

mod.getVerilogString

It even won’t be able to execute getVerilogString, which means we have no way to guard here.
Is possible for Scala to catch what has been imported, by reflection or macro? I know python can runtime do this, but I’m not sure can Scala achieve too.
Basically, My idea is monkey patching Module to catch this error and throw it.

I'm even fine with just putting them directly in chisel3 if we're really trying to make this as simple as possible. I'm open to being convinced otherwise though.

So can I PR a build system refactor? to merge those projects into one? ;p

[schoeberl]

I agree that this error is not something we want. Didn't know about it. Therefore, I put back the Chisel3 object variant. As the chisel3package lives in core we cannot put the method there. And we cannot add another chisel3 package in src/main.

From the usability point of view I think Chisel3.emitVerilog(new Foo()) is concise enough.

[sequencer]

I think before we merging this. We need get stage into core package.

[schoeberl]

@sequencer moving stage into core might be a good idea anyway. However, this is independent of this PR. No need to delay this one. Would like to have it in for 3.5 and the 3rd edition of the Chisel book.

[sequencer]

Sorry I thought I mis-approved yesterday.

stale

[schoeberl]

Can anyone please squash and merge, as I do not have write access to this repo.

Please squash this PR. It contains several commits that were for the discussion of the API only.

[schoeberl]

Maybe also backport to 3.4.4. Then I can start earlier to change my example code and the Chisel book.

[sequencer]

I’ll squash this with git cli today, I was terrified by buttons from GitHub websites, so not dare to push them. :(

[schoeberl]

I used the squash and merge button at GitHub and it worked nicely. Easier than doin it with native git.

…

On 16 Jun, 2021, at 1:39, Jiuyang Liu ***@***.***> wrote: I’ll squash this with git cli today, I was terrified by buttons from GitHub websites, so not dare to push them. :( — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1921 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE63GDLDQILFDW2A3G7BL3TS7QD7ANCNFSM45DKUKBA>.

[ekiwi]

Why do the two methods take different arguments? It seems to me like whether I want a string directly or generate a file, I should be able to at least pass annotations to both functions.

[ekiwi]

The other question is whether we should actually nudge people to use the SystemVerilog emitter instead of the Verilog emitter. The Verilog emitter won't generate cover/assert/assume statements even though they are supported by all tools that I am aware of.

[sequencer]

@ekiwi I think we can rediscuss this in the dev-meeting.