Mlp example #172
Mlp example #172
Conversation
* new function to update meta after quantization [only works on fixed] * comment out manual precision & dtype assignment --------- Co-authored-by: Jianyi Cheng <[email protected]>
…side hardware components
* Created test case with errors * cache quantized weight after the first inference * Removed quantization check --------- Co-authored-by: Cheng Zhang <[email protected]>
There was a problem hiding this comment.
Then changes to chop/models/manual seem fine (although maybe unnecessary with the new HFTracer update), but I almost all the changes in chop/passes/graph/analysis (i.e. the test_verilog pass) are reimplementations of functionality already existing in chop/passes/graph/transforms/verilog, but less general, with no tests under machop/test to evaluate, and using deprecated components. The same goes for all the changes in mase_components. I agree with verilog testing being an analysis pass, but I think it would be best to drop these changes and move the existing code to passes/graph/analysis instead. The refactoring of emit verilog needs work and could have been implemented without adding 1000 lines of code. I would suggest refactoring this feature since there is time and this feature is not time pressured.
|
|
||
|
|
||
| # DUT test specifications | ||
| class VerificationCase: |
There was a problem hiding this comment.
verification classes should inherit from mase_cocotb.testbench.Testbench
| dut.data_in_0_valid.value = 0 | ||
| dut.data_out_0_ready.value = 1 | ||
| debug_state(dut, "Pre-clk") | ||
| await FallingEdge(dut.clk) |
There was a problem hiding this comment.
Driving handshakes manually by assigning signals and awaiting clock edges is too verbose and error prone. We should use mase_cocotb.interfaces.streaming.StreamDriver instead
|
|
||
|
|
||
| @cocotb.test() | ||
| async def test_top(dut): |
There was a problem hiding this comment.
This functionality is already implemented in chop.passes.graph.transforms.verilog.emit_tb
|
|
||
| def get_test_parameters(mg): | ||
| def hardware_reshape(input_data, input_shape, tiling): |
There was a problem hiding this comment.
A good implementation is already available in chop.mase_cocotb.utils.fixed_preprocess_tensor
|
|
||
| class VerificationCase: |
There was a problem hiding this comment.
As previously mentioned, verification classes should inherit from mase_cocotb.Testbench
| return parameter_map | ||
|
|
||
|
|
||
| def runner(mg, project_dir, top_name): |
There was a problem hiding this comment.
Already implemented in mase_cocotb.runner
| total_size = math.prod( | ||
| node.meta["mase"].parameters["common"]["args"][param_name]["shape"] | ||
| ) | ||
|
|
||
| dim = len(node.meta["mase"].parameters["common"]["args"][param_name]["shape"]) |
There was a problem hiding this comment.
The main-adls-2324 branch has a more clean, updated implementation of this handling of the size/dim/depth for the emitted BRAM. I would merge that into mlp_example branch and then see if any further changes are required here
There was a problem hiding this comment.
Separating the dataflow emitter will be useful (to enable using a memory shell later on instead of local BRAM), but it seems like the way it's been implemented has a lot of repeated and redundant code without any inheritance, and the size of the file has gotten huge. I would refactor this so the SignalEmitter, InterfaceEmitter etc are all agnostic of whether it's dataflow or memory mapped
There was a problem hiding this comment.
This is an old implementation of linear, which should not be merged back into main. The new implementation without parametrization restrictions is already available in main-adls-2324, which is ready to be merged
There was a problem hiding this comment.
I don't understand any of the changes here. We're reverting back to using deprecated tb components, without any new functionality, when the current implementation is already working and general. I would discard this entire file
No description provided.