Remove implicit width truncation / Don't repeat Verilog's assignment issue in Chisel

[schoeberl]

I just realized that assignment of a larger width signal to a small width signal results in a silent truncation in Chisel. This is one of the worst issues in Verilog and modern synthesize tools should issue a warning as such an assignment is usually a bug.

Please, please, do not allow this in Chisel! The current behavior is even worse than Verilog as the generated Verilog code will NOT generate the warning.

Just had a meeting with a HW design company about presenting Chisel to them as they might need to switch language after a merger. And this Verilog assignment issue was their main argument against Verilog! So they would then not accept Chisel as well.

Type of issue: bug report

Impact: API modification (but a reasonable one)

[ducky64]

Although this never made it to a GitHub issue, there was some discussion during the 4/19 chisel-dev meeting about stricter truncation rules.

Ideas brainstormed:

• Need to ensure backwards compatibility somehow.
• Opt-in options: scope? WithStrictConnect mixin? separate strong-connect operator? import chisel3.strict? aspects?
• Implementation: possibly a strict connect operation in FIRRTL?

For what it's worth, the resolution was for someone to write a RFC, which apparently never happened.

[seldridge]

So, the super cool thing about Chisel is that you're not locked into this behavior. Consider the following:

class Foo extends RawModule {
  val a = IO(Input(UInt(4.W)))
  val b = IO(Output(UInt(3.W)))
  b := a
}

This gives you the following Mid FIRRTL:

circuit Foo :
  module Foo :
    input a : UInt<4>
    output b : UInt<3>
  
    skip
    b <= a

And the following Low FIRRTL (with the silent truncation):

circuit Foo :
  module Foo :
    input a : UInt<4>
    output b : UInt<3>
  
    b <= bits(a, 2, 0)

If this was something that a company really cared about, it is totally reasonable to write a custom FIRRTL transform (or for the Chisel/FIRRTL devs to make this available that can be optionally used) that will either print a warning or throw an error. So, you get the best of all worlds. With Verilog, it's whatever the standard says and whatever the vendor decides to implement.

[aswaterman]

I think there's general agreement that was a short-sighted decision, but it's too pervasive to change. I don't think mandatory warnings are a practical approach either, also because of pervasiveness in existing code.

chisel3.strict and/or a Firrtl transform both seem like viable solutions.

[ccelio]

I agree that this is an issue and some sort of opt-in system (I really like importing chisel3.strict) is needed. The more lint-like things we can move into chisel-time the better we are over verilog! =)

[jackkoenig]

As much as I hate the similarity to use strict in Perl and Javascript, I also vote for import chisel3.strict, I think before adding it we want to do a really good audit to see what things should be included.

My thought on this from the FIRRTL perspective is that we should just change FIRRTL to make <= strict so that when other frontends emit FIRRTL they get the more desirable behavior. From the Chisel perspective, we just emit <- for everything. Since import chisel3._ already splits connections out into leaf-level <= connections, we can just switch all of them to <- and thus the normal chisel3._ connection semantics (strict in the sense of fields much match, weak in the sense of implicit truncation) remain the same.

[jackkoenig]

(Or perhaps if we want to add this sooner, we make it import chisel3.experimental.strict so that users just accept that new things may be added 😈 )

[seldridge]

Width inference should relegate this to a FIRRTL-time check, yes?

Changing FIRRTL semantics to be strict seems to make sense to me. Similarly, enabling a new check that runs by default would necessitate a change to the spec. I do get a little nervous about allowing (compiler) front-end developers to control middle-end checks based on IR emission, however. I'd prefer explicit middle-end options/annotations.

Basically, I'd like the following behavior:

firrtl -Wall -Werror             # turn on all warnings, all warnings are errors
firrtl -Werror=silent-truncation # turn on silent truncation warnings
firrtl -Wno-silent-truncation    # turn off silent truncation warnings

The set of checks enabled by default would be defined by the spec. Warnings would be other things indicating potential user error and can be enabled/disabled with the above CLI that is backed by execution of specific transforms.

Concretely, the above options amount to porcelains around the following:

• RunFirrtlTransformAnnotation(new CheckSilentTruncation)
• CheckSilentTruncationOptions(fatal: Boolean)

I've done some playing around with the dependency API and turning the existing checks on/off or running checks more aggressively (e.g., I would like to be able to either (1) not run CheckTypes, (2) run CheckTypes only once, or (3) run CheckTypes after every InferTypes). I got stuck with that, but I think @jackkoenig's testing of the dependency API led to a bug fix that makes this possible.

[schoeberl]

On 22 Aug, 2019, at 23:34, Andrew Waterman [email protected] wrote:

I think there's general agreement that was a short-sighted decision, but it's too pervasive to change. I don't think mandatory warnings are a practical approach either, also because of pervasiveness in existing code.

Is it really so pervasive? Do people really write such code? I would consider this a design error in user code. What about checking the Rocket code base how often this really happens? I think this braking change would be easy to fix in user code and would improve the code quality.

BTW, partial assignment was also dropped form Chisel 2 to Chisel 3 with a argument that this is wrong code, but that broke some of my code (which I considered not wrong code ;-)

chisel3.strict and/or a Firrtl transform both seem like viable solutions.

Agree chisel3.strict would be a solution (work around) as this is easy for the Chisel user. A Firrtl transformation sounds a little bit technical for the user.

Cheers, Martin

[jackkoenig]

Is it really so pervasive? Do people really write such code? I would consider this a design error in user code. What about checking the Rocket code base how often this really happens? I think this braking change would be easy to fix in user code and would improve the code quality.

I did check (well SiFive's codebase including rocket-chip under our normal regression parameterizations) and it is pervasive. It's not an easy fix because it's not something that is caught by the Scala type system; it's only caught if you pick the right parameterization. I wrote tools to collect where it was occurring (in those parameterizations) and did an audit of the code using it, including asking the writers of those lines--they said it was intentional.

BTW, partial assignment was also dropped form Chisel 2 to Chisel 3 with a argument that this is wrong code, but that broke some of my code (which I considered not wrong code ;-)

We should have dropped implicit truncation between Chisel 2 and Chisel 3, but we didn't. This is about API stability; once we cracked 3.0.0 there is a duty to users not to break their code. I am personally very against implicit truncation, but am even more against breaking backwards compatibility. I don't think import chisel3.strict is great, but in my opinion, it's the best solution presented so far.

[schoeberl]

On 24 Aug, 2019, at 3:05, Jack Koenig [email protected] wrote:

Is it really so pervasive? Do people really write such code? I would consider this a design error in user code. What about checking the Rocket code base how often this really happens? I think this braking change would be easy to fix in user code and would improve the code quality.

I did check (well SiFive's codebase including rocket-chip under our normal regression parameterizations) and it is pervasive. It's not an easy fix because it's not something that is caught by the Scala type system; it's only caught if you pick the right parameterization. I wrote tools to collect where it was occurring (in those parameterizations) and did an audit of the code using it, including asking the writers of those lines--they said it was intentional.

Thanks for clarifying and checking. I am very surprised that someone is using truncation intentional. I can hardly come up with a use case. And if so, using explicit subfield expression would better show the intention. Ok, I accept that SiFive’s codebase rules here ;-)

I know that Scala’s type system is not so much used in Chisel. Maybe intentional, maybe because it would be too restrictive. But I don’t know the history of Chisel enough.

BTW, partial assignment was also dropped form Chisel 2 to Chisel 3 with a argument that this is wrong code, but that broke some of my code (which I considered not wrong code ;-)

We should have dropped implicit truncation between Chisel 2 and Chisel 3, but we didn't. This is about API stability; once we cracked 3.0.0 there is a duty to users not to break their code. I am personally very against implicit truncation, but am even more against breaking backwards compatibility. I don't think import chisel3.strict is great, but in my opinion, it's the best solution presented so far.

+1 for chisel.strict. Would be a default in all my Chisel files.

Cheers, Martin

[ccelio]

Somewhat related, we found an issue raised by a Verilog linter with myvec(idx) where idx was purposefully one bit larger than log(myvec.size). The wrap-around case was a carefully crafted DontCare, but it would be nice for these types of issues to be caught earlier before making it to a Verilog linter and the designer should specify "I really meant to be clever here!" (why do Verilog linters cost $$$??! :rage:)

Back on topic, I don't think I have a strong opinion on whether this should be a code import, a chisel compiler flag, or a firrtl flag. I can see an argument being made that creating a chisel3.strict will simply move the goal posts on "legacy" and we'll have to re-litigate this argument on the next language design ~mistake~ issue we run into (although I kind of like it when the designer tells me in his code what his assumptions where when he wrote the code).

C has -std, -Wall, etc., in their compiler flags, but that's pretty heavy-handed across everything being compiled, no? It seems we need individual module/file control on the compiler flags (which is already a chisel capability that we use for things like strict IO assignments). Do other languages/linter tools provide individual file/class/module control outside of the code file itself (although yuck on a waiver file!)?

[aswaterman]

I agree it can’t be a global setting because it that would preclude mixing code written under the old regime while getting better error checking for new code.

Re: other languages: It’s not unusual for large C projects to have some parts compiled with stricter flags than other parts, because large C projects are often decomposed into separately compiled libraries.

On Sat, Aug 24, 2019 at 5:40 PM Christopher Celio [email protected] wrote:

Somewhat related, we found an issue raised by a Verilog linter with myvec(idx) where idx was purposefully one bit larger than log(myvec.size). The wrap-around case was a carefully crafted DontCare, but it would be nice for these types of issues to be caught earlier before making it to a Verilog linter and the designer should specify "I really meant to be clever here!" (why do Verilog linters cost $$$??! 😡)

Back on topic, I don't think I have a strong opinion on whether this should be a code import, a chisel compiler flag, or a firrtl flag. I can see an argument being made that creating a chisel3.strict will simply move the goal posts on "legacy" and we'll have to re-litigate this argument on the next language design mistake issue we run into (although I kind of like it when the designer tells me in his code what his assumptions where when he wrote the code).

C has -std, -Wall, etc., in their compiler flags, but that's pretty heavy-handed across everything being compiled, no? It seems we need individual module/file control on the compiler flags (which is already a chisel capability that we use for things like strict IO assignments). Do other languages/linter tools provide individual file/class/module control outside of the code file itself (although yuck on a waiver file!)?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/freechipsproject/chisel3/issues/1163?email_source=notifications&email_token=AAH3XQWWUWWNV5ANNWPPWYLQGHIGFA5CNFSM4IOZGJQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CJ5SQ#issuecomment-524590794, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH3XQVY3KY5Q2AB632CJYLQGHIGFANCNFSM4IOZGJQQ .

[seldridge]

Note: that because options are just a means to generate Annotations, it's really the Annotations that are controlling this in any stage/phase/transform downstream of when the Annotation was created. We then have at least a language for expressing strictness at arbitrary granularities that is preserved across circuit transformations (though fine granularity would benefit from Chisel-level annotators as opposed to baroque options generating annotations).

[ducky64]

This was discussed at the dev meeting, with ideas and concerns but no resolution.

• General feeling from this thread: we want to do this, but backwards code compatibility is an issue.
• chisel3.strict seems fine, but there is the concern that the next time we make a change, we'll need to create a bunch of new imports, eg chisel3.strict2, chise3.strict3, ad nauseum. As a solution, it doesn't scale, unless we're sure this is the last breaking change. This also potentially raises discoverability issues - the best and newest is essentially hidden instead of being the default.
• There was discussion on whether strict flags should be at the file-level (import) or project-level (compiler flags). A potential implementation path may be the CompileOptions infrastructure.
• An alternate solution might be more like Scala's approach, which is that API changes may break code, but there is a compatibility option (like -Xsource=...). Essentially, when you bump your chisel version, you might get a bunch of code breakages, in which case you can either fix them, or add a compatibility flag.
• Another similar idea was just for users not to upgrade the version of chisel they're building against, but it's probably not a good idea since we generally don't support old versions or do backports.
• The FIRRTL implementation is also an open question. Is it a good idea to have this be a transform, since it kind of affects circuit correctness?

[aswaterman]

I think we want a solution that allows people to mix both loose and strict code, so that people can write new code in the new regime while being able to use older libraries that conform to the old regime.

On Mon, Aug 26, 2019 at 2:38 PM Richard Lin [email protected] wrote:

This was discussed at the dev meeting, with ideas and concerns but no resolution.

• General feeling from this thread: we want to do this, but backwards code compatibility is an issue.
• chisel3.strict seems fine, but there is the concern that the next time we make a change, we'll need to create a bunch of new imports, eg chisel3.strict2, chise3.strict3, ad nauseum. As a solution, it doesn't scale, unless we're sure this is the last breaking change. This also potentially raises discoverability issues - the best and newest is essentially hidden instead of being the default.
• There was discussion on whether strict flags should be at the file-level (import) or project-level (compiler flags). A potential implementation path may be the CompileOptions infrastructure.
• An alternate solution might be more like Scala's approach, which is that API changes may break code, but there is a compatibility option (like -Xsource=...). Essentially, when you bump your chisel version, you might get a bunch of code breakages, in which case you can either fix them, or add a compatibility flag.
• Another similar idea was just for users not to upgrade the version of chisel they're building against, but it's probably not a good idea since we generally don't support old versions or do backports.
• The FIRRTL implementation is also an open question. Is it a good idea to have this be a transform, since it kind of affects circuit correctness?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/freechipsproject/chisel3/issues/1163?email_source=notifications&email_token=AAH3XQTVKEVHAHRFDTDE3MDQGREMLA5CNFSM4IOZGJQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5FYOKA#issuecomment-525043496, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH3XQQ6FJDQVC5RGXVKR63QGREMLANCNFSM4IOZGJQQ .

[ccelio]

Thanks for the summary.

chisel3.strict seems fine [...] unless we're sure this is the last breaking change.

This is definitely an issue. I believe we are going to continue to find new ways to write broken code for quite a while and we will want to add additional protections to strict.

A potential implementation path may be the CompileOptions infrastructure.

Are there any concerns about CompileOptions? That's the current method for controlling strictness and it's a great way to slowly port legacy code to new standards.

I would really like to avoid any mechanism that 1) encourages legacy code to stay stuck in legacy, or 2) puts additional burden on the build system to manage libs/dependencies/compatibility-versioning (i.e., breaking apart a project into compatibility islands of strictness modes and linking together later).

The CompileOptions seems to have worked well so far for mixing Chisel2, looser Chisel3, and stricter Chisel3 semantics together in one build system.

[ducky64]

I'm not sure how it would be implemented yet, but the idea for chisel3._ being latest and you have to specifically add compatibility flags would mean the flags need to apply on a per-project (think sbt project, not top chip design) basis. We can certainly do a per file basis (change all import chisel3._ to import chisel3.compatibility_3_1 or something) but that's potentially a lot of code changes (even if trivially automate-able) per version bump.

I don't think there have been much concerns about CompileOptions as an infrastructure. I don't particularly want to expose it as a user-visible API though, since I'm not a fan of mix-and-match styles, but it should be fine as an implementation strategy for chisel3.strict or chisel3.compatibility.

I'm not sure what the solution for legacy code staying in legacy is, the general feeling I get is that refactoring for cleanliness generally isn't prioritized. I don't think anyone is actively campaigning that they like the old semantics, it more that no one wants to put resources into clean up. I think some Berkeley folks put a small effort into porting parts of rocket to chisel3, but that died from lack of interest? @chick might know more about that particular story?

[schoeberl]

On 27 Aug, 2019, at 0:00, Christopher Celio [email protected] wrote: Are there any concerns about CompileOptions? That's the current method for controlling strictness and it's a great way to slowly port legacy code to new standards.

Yes, it makes the build process more heavyweight, especially for beginners. I am really like import chisel3.strict. And adding later more restrictions to strict is OK, I think. When you use strict, you know that you use the latest, greatest save version of Chisel to protect you.

Martin

[albert-magyar]

And HDL developers will already have lots of experience with use strict; use warnings :)

[Martoni]

I strongly agree with @schoeberl . Import chisel3.strict should be a good solution. Is there a roadmap to integrate this functionality ?

[ducky64]

One issue with chisel3.strict is how that would scale as we realize other HDL constructs are antipatterns. Would chisel3.strict continually break code as we disallow more unsafe things? Or is it a one-shot thing (which seems short-sighted)? Or is there some middle ground?

[Martoni]

Would chisel3.strict continually break code as we disallow more unsafe things?

I think yes, but it's my opinion ;) If we don't want to "break" old code with chisel3.strict we don't use it.

[schoeberl]

On 18 Dec, 2019, at 20:38, Richard Lin [email protected] wrote:

One issue with chisel3.strict is how that would scale as we realize other HDL constructs are antipatterns. Would chisel3.strict continually break code as we disallow more unsafe things? Or is it a one-shot thing (which seems short-sighted)? Or is there some middle ground?

I am fine with breaking code when adding restrictions. But we should discuss those additions ;-)

Martin

[schoeberl]

As of telco 2/3: reduce operations (e.g., andR, orR) with zero width wires (signals) shall result in a runtime excpetion.

[aswaterman]

I'd recommend against that course of action. They're perfectly well-defined, and adding constraints like this increases boilerplate in generator code.

[schoeberl]

This is part of the strict discussion. That means this error comes only when including chisel3.strict, not in plain Chisel code.

[jackkoenig]

I'm not sure I agree it makes sense for strict either though, as Andrew said, it is perfectly well-defined and IMO intuitive since it matches the definition of the same concepts on empty collections

println("empty forall = " + Seq().forall { _ => false })
println("empty exists = " + Seq().exists { _ => true })

prints

empty forall = true
empty exists = false

[seldridge]

I don't like the argument of forall or exists being the reason why this should work. My concern is that this is a "reduction" and reductions (Scala's reduce or Haskell's fold1) error when given an empty list. @aswaterman has brought up a separate point that Seq.empty[Int].sum == 0. So, :woman_shrugging:.

I'd then argue that it's confusing that "reduction" in Chisel3 is really "fold with an initial value". I'm fine with this as it makes sense for generators, but I'm not big on the name as it is incongruous with what software engineers expect (and I realize that Chisel3 API decisions regarding default non-expanding addition was made such that it is what hardware engineers expect).

I'd probably propose the following:

1. Reduction operators in FIRRTL need to be explicitly defined on how they operate on zero-width wires.
2. I'd be in favor of having FIRRTL reduction operators error on zero-width wires.
3. The Chisel3 API for reduction operators should use FIRRTL reductions as opposed to custom reduction chains or muxes.
4. Chisel3 API may implement the reductions as folds with initial values and we document it as such.
5. I'd propose then that chisel3.strict doesn't include andR and instead uses foldAndTrue and foldOrFalse.

For (4), what I mean is val foo = bar.andR could emit as:

node foo = andr(cat(UInt<1>(1), bar))

[schoeberl]

What would be the result of an empty Wire on xorR?

I understand that some generators might emit zero-width wires. Is this a good generator? In most cases, I think a zero width wire (or register) is an error. Therefore, I would like that the strict annotation will catch this.

[aswaterman]

In general, the reductions should adhere to the axiom red(a ## b) === red(red(a) ## red(b)), so xorR should return 0 in this case.