pymtl / pymtl3

Pymtl 3 (Mamba), an open-source Python-based hardware generation, simulation, and verification framework
BSD 3-Clause "New" or "Revised" License
388 stars 46 forks source link

Improve index handling in Bits and Signal classes #275

Closed KelvinChung2000 closed 5 months ago

KelvinChung2000 commented 6 months ago

Enable syntax like a[0:] and a[:n], an often useful feature.

Please let me know if any other place will require such a change.

yo96 commented 6 months ago

Thanks for the PR. Looks good to me! Can you add a unit test in pymtl3/datatypes/test/bits_test.py? And perhaps a unit test for Connectables in pymtl3/dsl/test/Slicing_test.py?

KelvinChung2000 commented 6 months ago

The test case in Slicing_test.py is failing, but I cannot debug the problem. To me, it looks like the s._dsl.all_upblk_writes cannot find variables that are in the form of A[:16] as I am getting the following error with the test

pymtl3/dsl/test/Slicing_test.py:181: in test_write_two_slices_and_bit_without_specifying_width
    _test_model(Top)
pymtl3/dsl/test/Slicing_test.py:24: in _test_model
    A.elaborate()
pymtl3/dsl/ComponentLevel2.py:638: in elaborate
    s._elaborate_collect_all_vars()
pymtl3/dsl/ComponentLevel3.py:785: in _elaborate_collect_all_vars
    s._check_valid_dsl_code()
pymtl3/dsl/ComponentLevel3.py:745: in _check_valid_dsl_code
    s._check_upblk_writes()
pymtl3/dsl/ComponentLevel2.py:409: in _check_upblk_writes
    raise MultiWriterError( \
E   pymtl3.dsl.errors.MultiWriterError: Multiple update blocks write s.A.
E    - up_wr_16_32 at s
E    - up_wr_0_16 at s

As you can see it consider s.A[:16] and s.A[16:] both being s.A

yo96 commented 6 months ago

The test case in Slicing_test.py is failing, but I cannot debug the problem. To me, it looks like the s._dsl.all_upblk_writes cannot find variables that are in the form of A[:16] as I am getting the following error with the test

pymtl3/dsl/test/Slicing_test.py:181: in test_write_two_slices_and_bit_without_specifying_width
    _test_model(Top)
pymtl3/dsl/test/Slicing_test.py:24: in _test_model
    A.elaborate()
pymtl3/dsl/ComponentLevel2.py:638: in elaborate
    s._elaborate_collect_all_vars()
pymtl3/dsl/ComponentLevel3.py:785: in _elaborate_collect_all_vars
    s._check_valid_dsl_code()
pymtl3/dsl/ComponentLevel3.py:745: in _check_valid_dsl_code
    s._check_upblk_writes()
pymtl3/dsl/ComponentLevel2.py:409: in _check_upblk_writes
    raise MultiWriterError( \
E   pymtl3.dsl.errors.MultiWriterError: Multiple update blocks write s.A.
E    - up_wr_16_32 at s
E    - up_wr_0_16 at s

As you can see it consider s.A[:16] and s.A[16:] both being s.A

Hmm this can be tricky. I am looking into this. Might be related to the AST parsing logic.

yo96 commented 6 months ago

https://github.com/pymtl/pymtl3/blob/2eb57d3c4ca87d7fdc3fda467a9a9dca294c2c75/pymtl3/dsl/AstHelper.py#L64-L65 I think you may also need to modify this (and also in the starting_py39 function) to allow up and low to be None. Let me know if this works!

KelvinChung2000 commented 6 months ago

The test is passing now, thanks.