chisel refactoring to enable split main <-> test #104

jorendumoulin · 2024-05-08T08:38:15Z

See here my proposals to be able to split testing and source code without errors.
This includes the following:

remove the emitVerilog statements: they should be implemented by the user of the streamer with the correct parameters (like it is done in snax: Add wrapper and template generation #100). If these are useful for testing and we still want to keep them somewhere, I can reimplement them in a test.
removed most of the default argument assignments, as most of them held an arbitrary value (defaults should only be set if this value will be used in almost all cases)
some slight argument renaming to enable the split

rgantonio

I feel like it does look cleaner because all test files are collated (collected/aggregated). My only worry is:

Will it break when I generate new parameters? I left some comments on parts that i think are critical.
Will it also break @xiaoling-yi 's flow? Haha, make sure to discuss together and meet half-way 😄

hw/chisel/src/test/scala/snax/csr_manager/CsrManagerGenerate.scala

hw/chisel/src/test/scala/snax/streamer/StreamerTopGenerate.scala

hw/chisel/src/test/scala/snax/csr_manager/CsrManagerGenerate.scala

rgantonio · 2024-05-08T10:18:36Z

hw/chisel/src/test/scala/snax/streamer/StreamerTopGenerate.scala

For this file too make sure to add those I mean one of those and test if they generate properly: sbt "runMain snax.streamer.MacStreamerTop "

// streamertop for MAC object MacStreamerTop extends App { emitVerilog( new StreamerTop( StreamerParams( temporalAddrGenUnitParams = MacStreamerParameters.temporalAddrGenUnitParams, fifoReaderParams = MacStreamerParameters.fifoReaderParams, fifoWriterParams = MacStreamerParameters.fifoWriterParams, stationarity = MacStreamerParameters.stationarity, dataReaderParams = MacStreamerParameters.dataReaderParams, dataWriterParams = MacStreamerParameters.dataWriterParams ) ), Array("--target-dir", "generated/streamertop/mac") ) }

See comment above!

xiaoling-yi · 2024-05-08T11:57:21Z

hw/chisel/src/test/scala/snax/streamer/StreamerTopGenerate.scala

+    Array("--target-dir", "generated/")
+  )    
+
+}


I vote to put the parameter example for instancing gemm and simd back. We can write some comments in the code to say this example configuration is for which accelerator. Because I think 1) these example configurations are good for people to understand how to configure the streamer for different use cases as the configuration format of the streamer is quite complicated. 2) making sure the streamer still works after any modification for these configurations is essential. 3) These configurations show the streamer's reusability, working for different accelerators.

Alright! As discussed: we like to avoid having code that is not used anywhere. Therefore the example configuration is displayed in the added documentation. The tests for different configurations are run through the automatic generation of scala parameters using the .hjson configuration files! For now this is only ALU but when the other accelerators are added these will be tested as well!

xiaoling-yi

Overall this PR looks good to me, I just have some small comments. Please kindly check it!

xiaoling-yi · 2024-05-08T16:52:31Z

hw/chisel/.gitignore

@@ -0,0 +1,5 @@
+generated
+project


Actually, two files in the ./hw/chisel/project folder is used, which are plugins.sbt and build.properties

Aha, yes I missed this, I excluded those two files!

xiaoling-yi · 2024-05-08T16:56:52Z

hw/chisel/doc/streamer.md

+
+<p align="center">
+  <img src="./docs/microarch.svg" alt="">
+</p>


This figure is disappeared!

I re-added it!

xiaoling-yi · 2024-05-08T16:59:08Z

hw/chisel/doc/streamer.md

+| tcdm_req | write | io_data_tcdm_req_0_bits_write | 1| Out| The signal indicates this request is for CSR write or read. |
+| tcdm_req | valid | io_data_tcdm_req_0_valid | 1| Out| The signal indicates if this request is valid. |
+| tcdm_req | ready | io_data_tcdm_req_0_ready | 1| Int| The signal indicates if the TCDM is ready for this CSR request. |
+| | . | . | . | . | There can be a large number of tcdm_req ports depending on the spatial unrolling factors for the data readers and the data writers. tcdm_req ports for readers have lower index number. A detailed mapping for the tcdm_req ports and the data mover ports can be found at `Streamer.scala` line 269-311.|


The line number is 330-372 now. Or we can delete the line number here.

Yes I just deleted it because it might change later

xiaoling-yi · 2024-05-08T17:00:07Z

hw/chisel/doc/streamer.md

+| offset + [0..temporalDim - 1]   |    temporalLoopBoundCSRs     | temporal loop bound for each temporal dimension.           |
+| offset + temporalDim + [0..temdataMoverNum * temporalDimporalDim - 1]   |    temporalLoopSrtidesCSRs     | temporal loop strides for each temporal dimension and for each data mover.          |
+| offset + temporalDim +  dataMoverNum * temporalDim + [0..spatialDim.sum - 1]   |    spatialLoopSrtidesCSRs     | spatial loop strides for each data mover and for corresponding spatial dimension. The spatial dimension for each data mover can be different. It depends on the accelerator.        |
+| offset + temporalDim +  dataMoverNum * temporalDim + spatialDim.sum + [0..dataMoverNum - 1]   |    basePtrCSRs     |  base pointers for each data mover.          |


Add the status CSR here too?

xiaoling-yi · 2024-05-08T17:10:26Z

hw/chisel/src/test/scala/snax/streamer/StreamerTopGenerate.scala

+    Array("--target-dir", "generated/")
+  )    
+
+}


Can we add the modular system verilog generation test back? It is useful for modular generation and tests.

What is the modular system verilog genration?

I mean the code like this:
emitVerilog( new SpatialAddrGenUnit(SpatialAddrGenUnitParams()), Array("--target-dir", "generated/streamer") )
for testing the system verilog genration for each module.

We do not want people to use subparts as systemverilog components, right?
The testing of systemverilog generation is done by the top module, the only one that we are going to use. All lower modules are included in this test. Having them separate is duplicated testing, and is confusing as we don't want to generate verilog for the modules separately. Does that make sense?

Ok, I understand your point of view. I am fine to merge this PR.

Ok, thank you!

rgantonio

I will give the go signal here but I prefer Xiaoling gives the final go signal since she uses this more closely than I do 😄

Otherwise, I am okay with it. Let's just make sure to fill-in the CSR manager docu.

rgantonio · 2024-05-08T19:44:06Z

hw/chisel/doc/microarch.svg

Do you think it's better we dump images in our repo? I have images that were dumped in a google drive then sent the link to the gh pages 😅 what do you think?

A lot of people seem to have a lot of opinions about this but in the end it doesn't seem to matter much 😅 . The only worry about Google Drive links is that if they disappear they are gone forever. Maybe the best way is including it as github user content, I'll change it to that!

rgantonio · 2024-05-08T19:45:36Z

hw/chisel/README.md

I'm actually surprised we put the README's in this PR already. For the CSR Manager we do that in a separate PR?

Yes, ideally it should've been included previously, but I never noticed. We need it here because the readme replaces some code that wasn't used and thus removed. For the CSR manager we indeed best separate it. Although is there already documentation for this? The streamer readme is just a copy from the old repo

I made a new issue #106 for this 😄

xiaoling-yi · 2024-05-09T17:33:46Z

hw/chisel/src/test/scala/snax/csr_manager/StreamerTestParameter.scala

}
+

Should the streamer test parameter be in the streamer folder?

Yes, this was an error. I fixed it!

xiaoling-yi · 2024-05-09T17:35:39Z

hw/chisel/src/test/scala/snax/streamer/StreamerTopGenerate.scala

+    Array("--target-dir", "generated/")
+  )    
+
+}


I mean the code like this:
emitVerilog( new SpatialAddrGenUnit(SpatialAddrGenUnitParams()), Array("--target-dir", "generated/streamer") )
for testing the system verilog genration for each module.

* dnn: Increase error threshold in FA-2 * dnn: Switch to naive FP32 as baseline fails in FlashAttention-2 * target: Re-introduce FlashAttention in CI --------- Co-authored-by: Viviane Potocnik <[email protected]> Co-authored-by: Luca Colagrande <[email protected]>

* light refactoring to enable split main <-> test * enable CI to actually run the Chisel unit tests * remove unused instance parameters * formatting * cleaning up some test params * resolve trailing space * add chisel .gitignore * add verilog generator scripts to test * use TestParameters * move test parameters back to /tests * add streamer documentation * rename documentation file * add chisel readme * add documentation image * update image path * address comments * update streamer.md image * Delete hw/chisel/doc/microarch.svg * move test parameters to correct folder

jorendumoulin marked this pull request as ready for review May 8, 2024 09:58

jorendumoulin requested review from JosseVanDelm, rgantonio and xiaoling-yi May 8, 2024 09:59

rgantonio reviewed May 8, 2024

View reviewed changes

xiaoling-yi reviewed May 8, 2024

View reviewed changes

jorendumoulin added 9 commits May 8, 2024 15:09

light refactoring to enable split main <-> test

f6b8a5a

enable CI to actually run the Chisel unit tests

2f4132a

remove unused instance parameters

3bb6d33

formatting

68572c0

cleaning up some test params

a42e5c2

resolve trailing space

f379d60

add chisel .gitignore

407b557

add verilog generator scripts to test

e12104f

use TestParameters

0aabecc

jorendumoulin force-pushed the joren/fix-chisel branch from 0b6d13d to 0aabecc Compare May 8, 2024 13:10

jorendumoulin added 4 commits May 8, 2024 15:13

move test parameters back to /tests

e10f33e

add streamer documentation

c1f3c71

rename documentation file

868a818

add chisel readme

67602b5

jorendumoulin requested review from xiaoling-yi and rgantonio May 8, 2024 13:56

xiaoling-yi reviewed May 8, 2024

View reviewed changes

jorendumoulin added 3 commits May 8, 2024 19:52

add documentation image

1513480

update image path

0cd1956

address comments

11d3da5

rgantonio reviewed May 8, 2024

View reviewed changes

jorendumoulin added 2 commits May 10, 2024 08:53

update streamer.md image

3180d72

Delete hw/chisel/doc/microarch.svg

c054ee8

xiaoling-yi reviewed May 10, 2024

View reviewed changes

move test parameters to correct folder

9b4649c

rgantonio assigned jorendumoulin May 10, 2024

rgantonio added the clean up To keep repos clean label May 10, 2024

xiaoling-yi approved these changes May 10, 2024

View reviewed changes

jorendumoulin merged commit 1285d77 into main May 10, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chisel refactoring to enable split main <-> test #104

chisel refactoring to enable split main <-> test #104

jorendumoulin commented May 8, 2024 •

edited

Loading

rgantonio left a comment

rgantonio May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 8, 2024 •

edited

Loading

jorendumoulin May 8, 2024

xiaoling-yi left a comment

xiaoling-yi May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 8, 2024

jorendumoulin May 8, 2024

xiaoling-yi May 9, 2024

jorendumoulin May 10, 2024

xiaoling-yi May 10, 2024

jorendumoulin May 10, 2024

rgantonio left a comment

rgantonio May 8, 2024

jorendumoulin May 10, 2024

rgantonio May 8, 2024

jorendumoulin May 10, 2024

rgantonio May 10, 2024

jorendumoulin May 10, 2024

xiaoling-yi May 9, 2024

jorendumoulin May 10, 2024

xiaoling-yi May 9, 2024

		@@ -0,0 +1,5 @@
		generated
		project

chisel refactoring to enable split main <-> test #104

chisel refactoring to enable split main <-> test #104

Conversation

jorendumoulin commented May 8, 2024 • edited Loading

rgantonio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoling-yi May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoling-yi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgantonio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorendumoulin commented May 8, 2024 •

edited

Loading

xiaoling-yi May 8, 2024 •

edited

Loading