rocket-chip icon indicating copy to clipboard operation
rocket-chip copied to clipboard

Published 20 hours ago • verified publisher icon chipsalliance

Port legacy code to chisel3

Open jiegec opened this issue 3 years ago • 11 comments

Related issue: None

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes

jiegec avatar May 12 '22 03:05 jiegec

Those modifications generally LGTM. But notice: changing Chisel to chisel3 not only affects the syntax, but also the CompilerOption, see https://github.com/chipsalliance/chisel3/blob/2b48fd15a7711dcd44334fbbc538667a102a581a/src/main/scala/chisel3/compatibility.scala#L19 Migration from NonStrict to Strict might introduce tricky issues. So I wanna hold this until we have more tests.

sequencer avatar May 12 '22 04:05 sequencer

I feel like this needs to be done procedurally. In place of tests, verifying that the generated verilog is formally equivalent (or ideally, identical), would be fine.

Is there a particular motivation for updating these, besides cleanliness?

jerryz123 avatar May 12 '22 05:05 jerryz123

Is there a particular motivation for updating these, besides cleanliness?

Nope. Cleanliness is important IMO.

jiegec avatar May 12 '22 05:05 jiegec

For new contributors, there are no documentation for legacy Chisel2 syntax. Which means the legacy codes is hard to understand for newbies. But the most important is the correctness. So this requires a persuasive test(In formal or a unittest).

sequencer avatar May 12 '22 06:05 sequencer

Cleanliness is important IMO.

Unfortunately, I think the uglier syntax stands until we prove or provide reasonable evidence that, using chisel3 and its compiler, the verilog IR is of equal or better value as a result of these changes

See Andrew's comment at your original PR: https://github.com/chipsalliance/rocket-chip/pull/2925#issuecomment-1018315787

michael-etzkorn avatar May 12 '22 14:05 michael-etzkorn

I tried to do a formal equivalance checking between the verilog code generated by this PR and the original code and found that this PR couldn't generate rtl correctly. @jiegec Could you reproduce my errors with following steps?

cd vsim
make verilog CONFIG=freechips.rocketchip.system.DefaultConfig

tianrui-wei avatar Aug 24 '22 00:08 tianrui-wei

What errors did you see?

jerryz123 avatar Aug 24 '22 15:08 jerryz123

Apologies for the false alarm, jerry helped me fix the problem. I'll try to do the verification later.

tianrui-wei avatar Aug 25 '22 00:08 tianrui-wei

@tianrui-wei can you take over sheperding this? Just make a new PR from this branch.

jerryz123 avatar Sep 14 '22 00:09 jerryz123 

val sourceTable = Wire(Vec(edgeIn.client.endSourceId, out.aw.bits.id))
sourceStall.foreach(_ := false.B)

I didn't think this related to Chisel._ -> chisel3._ porting? I'm not confident about this change, cc @jackkoenig for advices.

@tianrui-wei can check formal equivalency with JasperGold. From my brief glance, it seemed like some entries in that wire are never initialized or referenced

jerryz123 avatar Sep 14 '22 02:09 jerryz123

@tianrui-wei can check formal equivalency with JasperGold. From my brief glance, it seemed like some entries in that wire are never initialized or referenced

This is a little strange. I'm going back to chisel code base to find more clues.

sequencer avatar Sep 14 '22 02:09 sequencer