chipsalliance / silicon-notebooks

Apache License 2.0
161 stars 28 forks source link

xls-workshop-openlane: add pipeline assignement #36

Closed proppy closed 1 year ago

proppy commented 1 year ago

TEST=https://colab.research.google.com/github/proppy/silicon-notebooks/blob/add-pipeline/xls-workshop-openlane.ipynb

@t-sasatani @shige-compit

t-sasatani commented 1 year ago

@proppy Tried the pipeline part with pipeline_stages = 2 clock_period_ps = 1000 The wires associated with the bit_slice were named p1_bit_slice_34 and p1_bit_slice_35, not p1_bit_slice_33. The test and waveform preview worked when I changed the assert lines accordingly. I wanted to post this in case as I wasn't sure if this was intended.

https://github.com/t-sasatani/silicon-notebooks/blob/pipeline_test/xls-workshop-openlane.ipynb

t-sasatani commented 1 year ago

Also, the following error occurred when running !flow.tcl -design . -verbose 10 -to cts It ran until the end of the notebook anyway, and the preview showed up so might not be a problem but let me post it here. https://github.com/t-sasatani/silicon-notebooks/blob/pipeline_test/xls-workshop-openlane.ipynb

[ERROR]: There are hold violations in the design at the typical corner. Please refer to 'runs/RUN_2022.12.16_06.34.10/reports/synthesis/2-syn_sta.min.rpt'.

(This seemed to be a similar issue happening in MPW-6, but adding the GLB_RT_ADJUSTMENT in the config.json didn't work out.) https://github.com/The-OpenROAD-Project/OpenLane/issues/1090

proppy commented 1 year ago

The wires associated with the bit_slice were named p1_bit_slice_34 and p1_bit_slice_35, not p1_bit_slice_33. The test and waveform preview worked when I changed the assert lines accordingly. I wanted to post this in case as I wasn't sure if this was intended.

Ah good to know, we should add TODO so that people know they need to update those as well.

proppy commented 1 year ago

[ERROR]: There are hold violations in the design at the typical corner. Please refer to 'runs/RUN_2022.12.16_06.34.10/reports/synthesis/2-syn_sta.min.rpt'.

Yes, this is expected, you need to tune the CLOCK_PERIOD until you don't get any violation (and you meet timing: slack == MET).

Maybe we should make the instruction clearer?

proppy commented 1 year ago

The test and waveform preview worked when I changed the assert lines accordingly. I wanted to post this in case as I wasn't sure if this was intended.

Added instructions in b97683f.

proppy commented 1 year ago

Also, the following error occurred when running !flow.tcl -design . -verbose 10 -to cts

Added instructions on how to fix the error in 5b791bf (to make it clear that's part of the assignement).

proppy commented 1 year ago

@t-sasatani PTAL

t-sasatani commented 1 year ago

Ah good to know, we should add TODO so that people know they need to update those as well.

This also happens in the Preview of the waveform part. 'signal': list(signals_to_wave('user_module.vcd', top='top.user_module_tb', cycles = pipeline_stages+1, signals = ['io_in[7:0]', 'p1_bit_slice_33[3:0]', 'p1_bit_slice_34[3:0]', 'out[7:0]']))

Probably we shouldn't hide the codes in the preview block?

t-sasatani commented 1 year ago

Added instructions on how to fix the error in https://github.com/chipsalliance/silicon-notebooks/commit/5b791bf31e75b10b56580462f2fa83bfdc02fa95 (to make it clear that's part of the assignment).

Thanks, I think it's clear now. Might be more educative if we could have a look at the 2-syn_sta.min.rpt log at the front, but let's see what happens Monday.

proppy commented 1 year ago

This also happens in the Preview of the waveform part. 'signal': list(signals_to_wave('user_module.vcd', top='top.user_module_tb', cycles = pipeline_stages+1, signals = ['io_in[7:0]', 'p1_bit_slice_33[3:0]', 'p1_bit_slice_34[3:0]', 'out[7:0]']))

oh good point, I think I could rework the preview block so that it extract those from the vcd.

t-sasatani commented 1 year ago

I could rework the preview block so that it extract those from the vcd.

Oh that sounds great!

proppy commented 1 year ago

. Might be more educative if we could have a look at the 2-syn_sta.min.rpt log at the front, but let's see what happens Monday.

Yes, we should be able to add it to a cell if you want.

t-sasatani commented 1 year ago

I think the notebook is self-contained as it should be, but I have a feeling that some students might blindly change parameters instead of looking into the references and understanding because the time of the workshop is not so long. If there's a mention on signal timing when explaining pipelines, I think that's enough.

t-sasatani commented 1 year ago

If there's a mention on signal timing when explaining pipelines, I think that's enough.

Kosuge-sensei seems to be up for making this (looking at spaces)

proppy commented 1 year ago

I could rework the preview block so that it extract those from the vcd.

Done in c770020

proppy commented 1 year ago

Rebased, @shige-compit @t-sasatani does that look good to you?

proppy commented 1 year ago

Merging since the jp version is already commited.

t-sasatani commented 1 year ago

Rebased, @shige-compit @t-sasatani does that look good to you?

Yup, looks great!