Chisel Linting Framework

[azidar]

Type of change: new feature

Impact: API Addition

Development Phase: implementation

Release Notes

• Added CHISEL_OPTIONS environment variable
• Added FIRRTL_OPTIONS environment variable
• Added Chisel linting options (--lint, --lint-options, --lint-whitelist:anon-regs, --lint-whitelist:trunc-widths)

Summary

There have been many instances where consumers of rocket-chip Verilog have to run LEC tools, debuggers, physical design tools. When registers don't have stable names, changes to the generator cause changes to the names of registers, making the lives of those downstream more difficult.

This PR adds the following:

• A linting check which looks for unnamed registers and reports them to the user as an error
• A linting check for connections from a larger-width signal to a smaller-width signal

To try it out:

> cd emulator
> CHISEL_OPTIONS="--lint * --lint-options display:*=5" make
...
Exception in thread "main" freechips.rocketchip.linting.LintException: 
Lint rule anon-regs: 102 exceptions!
 - Recommended fix:
     Use named intermediate val, or if that fails use @chiselName or *.suggestName(...)
 - Whitelist file via Chisel cmdline arg:
     --lint-whitelist:anon-regs Arbiter.scala,AtomicAutomata.scala,BTB.scala,Broadcast.scala,CSR.scala,Counter.scala,Counters.scala,DCache.scala,Debug.scala,Deinterleaver.scala,Edges.scala,FIFOFixer.scala,FPU.scala,Fragmenter.scala,Frontend.scala,HellaCacheArbiter.scala,ICache.scala,Interrupts.scala,Monitor.scala,Periphery.scala,Plic.scala,Reg.scala,Replacement.scala,RocketCore.scala,RocketTile.scala,ShiftQueue.scala,SourceShrinker.scala,ToAXI4.scala,ToTL.scala,Valid.scala,Xbar.scala,package.scala
 - Whitelist file via Chisel scala API:
     whitelist("anon-regs", Set("Arbiter.scala","AtomicAutomata.scala","BTB.scala","Broadcast.scala","CSR.scala","Counter.scala","Counters.scala","DCache.scala","Debug.scala","Deinterleaver.scala","Edges.scala","FIFOFixer.scala","FPU.scala","Fragmenter.scala","Frontend.scala","HellaCacheArbiter.scala","ICache.scala","Interrupts.scala","Monitor.scala","Periphery.scala","Plic.scala","Reg.scala","Replacement.scala","RocketCore.scala","RocketTile.scala","ShiftQueue.scala","SourceShrinker.scala","ToAXI4.scala","ToTL.scala","Valid.scala","Xbar.scala","package.scala"))
 - Disable this linting check:
     Omit anon-regs from --lint option
 - Modify display settings with:
     --lint-options ...,display:anon-regs=<number>,...
anon-regs.1: @[Arbiter.scala 21:23]  in Set(TLXbar_7, AXI4ToTL, TLXbar_5, TLXbar, TLXbar_8)
anon-regs.2: @[Arbiter.scala 54:30]  in Set(TLXbar_7, AXI4ToTL, TLBroadcast, TLError_1, TLXbar_5, TLXbar, TLXbar_8, TLAtomicAutomata_1)
anon-regs.3: @[Arbiter.scala 79:26]  in Set(TLXbar_7, AXI4ToTL, TLBroadcast, TLError_1, TLXbar_5, TLXbar, TLXbar_8, TLAtomicAutomata_1)
anon-regs.4: @[AtomicAutomata.scala 76:28]  in Set(TLAtomicAutomata_1)
anon-regs.5: @[AtomicAutomata.scala 77:24]  in Set(TLAtomicAutomata_1)

Lint rule trunc-widths: 308 exceptions!
 - Recommended fix:
     Truncate width prior to connections
 - Whitelist file via Chisel cmdline arg:
     --lint-whitelist:trunc-widths BTB.scala,Broadcast.scala,BundleMap.scala,CSR.scala,Counters.scala,Custom.scala,DCache.scala,Debug.scala,Deinterleaver.scala,DivSqrtRecFN_small.scala,Edges.scala,FIFOFixer.scala,FPU.scala,Fragmenter.scala,Frontend.scala,HellaCache.scala,HellaCacheArbiter.scala,IBuf.scala,Monitor.scala,MulAddRecFN.scala,PMP.scala,PTW.scala,Plic.scala,PlusArg.scala,RegisterRouter.scala,RocketCore.scala,SimAXIMem.scala,SourceShrinker.scala,TLB.scala,ToAXI4.scala
 - Whitelist file via Chisel scala API:
     whitelist("trunc-widths", Set("BTB.scala","Broadcast.scala","BundleMap.scala","CSR.scala","Counters.scala","Custom.scala","DCache.scala","Debug.scala","Deinterleaver.scala","DivSqrtRecFN_small.scala","Edges.scala","FIFOFixer.scala","FPU.scala","Fragmenter.scala","Frontend.scala","HellaCache.scala","HellaCacheArbiter.scala","IBuf.scala","Monitor.scala","MulAddRecFN.scala","PMP.scala","PTW.scala","Plic.scala","PlusArg.scala","RegisterRouter.scala","RocketCore.scala","SimAXIMem.scala","SourceShrinker.scala","TLB.scala","ToAXI4.scala"))
 - Disable this linting check:
     Omit trunc-widths from --lint option
 - Modify display settings with:
     --lint-options ...,display:trunc-widths=<number>,...
trunc-widths.1: @[:[email protected]] _T_3704 <= _T_3702 // Connecting width 64 to width 16 in Set(CSRFile)
trunc-widths.2: @[:[email protected]] _T_3733 <= wdata // Connecting width 64 to width 32 in Set(CSRFile)
trunc-widths.3: @[:[email protected]] _T_3794 <= _T_3792 // Connecting width 64 to width 16 in Set(CSRFile)
trunc-widths.4: @[:[email protected]] _T_3978 <= _T_3976 // Connecting width 64 to width 8 in Set(CSRFile)
trunc-widths.5: @[:[email protected]] _T_3998 <= _T_3996 // Connecting width 56 to width 8 in Set(CSRFile)
Makefrag-verilator:16: recipe for target '/scratch/adami/work/rocket-chip-dev/rocket-chip-linting/emulator/generated-src/freechips.rocketchip.system.DefaultConfig.v' failed

To see the compiler options, run with:

> cd emulator
> CHISEL_OPTIONS="--help" make
...
freechips.rocketchip.linting.LintReporter
  --lint [*]|[<lintRule>,<lintRule>,...]
                           Enable linting for specified rules, where * is all rules. Available rules: anon-regs,trunc-widths.
  --lint-options (strict|warn)[,displayTotal=<numError>][,display:<lintName>=<numError>]
                           Customize linting options, including strict/warn or number of violations displayed.
freechips.rocketchip.linting.rule.LintAnonymousRegisters
  --lint-whitelist:anon-regs <filename1>.scala[,<filename2>.scala]*
                           Enable linting anonymous registers for all files except provided files.
freechips.rocketchip.linting.rule.LintTruncatingWidths
  --lint-whitelist:trunc-widths <filename1>.scala[,<filename2>.scala]*
                           Enable linting anonymous registers for all files except provided files.`1
...

TODO:

• Add scaladoc to all files
• Solicit feedback from the community on ideas/implementation

[jackkoenig]

This looks great overall. I would be more specific than just "linting" since there are other possible lint rules we may want to add some day. I would call this --lint-register-names or something like that.

[aswaterman]

Can't we fix 80+% of these with @chiselName instead of the more verbose suggestName approach?

[aswaterman]

... or perhaps the suggestName ones are the ones where @chiselName doesn't cut it.

[aswaterman]

rocket changes lgtm.

[jackkoenig]

... or perhaps the suggestName ones are the ones where @chiselName doesn't cut it.

Yeah I think @chiselName is missing them, bug to-be-fixed but no reason to suffer with bad register names in the meantime.

[hcook]

Seems like an ok way to document @chiselName failures; @azidar can confirm but I think he attempted @chiselName first in most cases.

[azidar]

Yeah, @chiselName didn't fix the majority of these. I think there is a serious bug, but in the meantime this PR highlights where we can fix these.

[mwachs5]

Thanks!
Your PR mentions modules, but I think you only fixed registers. Does that mean unnamed
Modules aren’t detected by the linter, or aren’t actually present?

In general I see the naming algorithm you applied, but in some cases when you had to make up a name you said “reg_foo” and in others “foo_reg”, not sure if that was intentional in those cases? FWIW I prefer the latter

[azidar]

Ok so my plan is to make this PR only add the Linting transform and options, but not run the linter by default and not include any of the name fixes so all the regressions pass. That will let us incrementally change names but still get this PR in.

[jackkoenig]

Generally looks good. A few nits but I'm okay with leaving them for later improvement

No longer including changes to rocketchip RTL

Major changes since review

[jackkoenig]

@azidar hey sorry I force pushed the chisel-3-3 branch so you'll need to rebase, I didn't realize this was against that branch. Why is this PRed against that branch instead of master? Does it rely on Chisel 3.3 stuff?

[jackkoenig]

LGTM, but please retarget against master