Emit Parametrized PeakRDL output

[galaviel]

Greetings ! Awesome project BTW!

I did a quick pilot in my company using PeakRDL with Synopsys VCS, replacing an existing register module with SystemRDL specification + PeakRDL output. Worked Nicely.

A feature that we miss is the ability to emit parametrized output. Meaning keep the symbolic SystemRDL parameters in the generated RTL instead of replacing them with actual values.

This can allow us to embed the generated RTL CSR module in a generic/parametrized design, have the user specify actual parameters in the instantiation, and let those Parameter values seep down to the generated CSR RTL. (in addition to configuring the logic that surrounds the register module).

This can be a killer feature.. Is this a change that can be done at the PeakRDL generator level? or is this in the SystemRDL Parser code/project before that?

Any help appreciated, I think I'll take a look myself..

Thanks in advance, Gal.

[amykyta3]

That is certainly the long-term plan, however it is not something that is possible currently until I make updates to the underlying compiler. For that, see: https://github.com/SystemRDL/systemrdl-compiler/issues/58

Since this is a request for the PeakRDL-regblock sub-project, I will migrate this issue there.

[galaviel]

Thanks allot! really looking forward to that. Note it will mean changing the port type from Struct's to SystemVerilog Interfaces since the former does not allow parametrization (as far as I can tell). Which results in many small changes to the output RTL.

[amykyta3]

Also looking forward to implementing this one! However it is easily one of the more complicated features I have on my list. Parameters can potentially influence all sorts of aspects of the register block, not just the number of elements in an array, or an address offset. There are countless edge cases such as property assignments and structural changes as well.

To avoid over-complicating, the implementation of this feature for the regblock will likely be limited to only be allowed for a select few value assignments that cover the more common use-cases. Off the top of my head:

• Field width
• Field reset value
• Array size
• Address offset (maybe)
• ispresent property

Beyond these, the implementation can get extremely complicated due to limitations in the SystemVerilog language.

Agree that something would have to change for the hwif definition to accommodate this.

[galaviel]

Sounds great! I think that's the way to go... Implementing the limited feature set users are actually likely to use, is a great way to provide real life value to users. fast. Regarding Array size - will you allow an expression? such that the size of the Array can be e.g. 4*PARAM ?

Another question: do you have any ETA on this? I think right now I'll try to assign unique values to parameters and try to search & replace them in the generated RTL, that might work for simple stuff and provide a simple work-around. Thanks!

[amykyta3]

Yes - in many of the cases, preserving the parameter to the output will necessitate preserving expressions as well.

Since this is something I work on as a hobby on my own personal time, it is difficult to provide an ETA. I have quite a few items in my backlog that I plan to address first before I start working on this feature.

[galaviel]

Thanks.. regarding ETA - of course, I understand and I greatly appreciate the awsome work you've done :)

I might try to look in the code as well. Coming from EE background I haven't studied compilers at all but I hope I can manage...

Again, a simpler work-around for now (since I do need this) would be to assign unique integer values to PARAMETERS, then perform search & replace in the genereated RTL. I think that might work mostly, perhaps a manual touch-up will be required in 2-3 places after that, which is hopefully an acceptable solution for my users.

Thanks again!

[galaviel]

Hi,

I'm experimenting with adding very limited support for parametrized out - only for register Array sizes and field reset values for now. I've modified all 3 repo's (the compiler, peakrdl command line tool and the regblock generator/exporter).

Surprisingly this seems to work. The output looks OK for my simple RDL input file.

The readback part is more difficult though. One thing I don't understand is the need for 'n_regs' in file peakrdl_regblock\readback\generators.py.

Since all register array dimensions are known in advance, why is it really necessary to go through all the individual register MDA array elements in order to dedude n_regs? this is known already..

Just curious

Thanks, Gal.

[amykyta3]

Part of the readback block is to reduce all possible readable registers into a single flattened array. Since not all registers are necessarily going to to be readable by software, this is not necessarily going to simply be the accumulation of all enclosed registers in an array. In order index into this flattened array, the cumulative number of elements prior needs to be tracked. This is done via the n_regs variable.

[galaviel]

Right. not all registers are readable. Got it, thanks.In that case I would perhaps suggest to rename n_regs -> n_readable_regs.

Also, related to naming convention, perhaps I got this wrong, but in file PeakRDL-regblock\src\peakrdl_regblock\forloop_generator.py,I see two classes: Body and LoopBody. It seems the latter is the entire loop rather than only the loop body as its name suggests - (includes the for(..) line, the body itself - assignments and the enclosing 'end'). Is my understanding correct? I think I'll finish this limited "sketch"/try with parametrization (the templates needs modifications too - need to change to SystemVerilog Interface instead of struct and many other small changes). After that I need to do some cleaning.  Is this work interesting in any way to post back to the community or contribute back to the project? I imagine you have your own ideas on how to implement this (most likely better than mine..) but I do really need this feature urgently...should I fork the project and commit there? haven't work with Github in the past..

On Monday, 17 April 2023 at 04:35:44 GMT+3, Alex Mykyta ***@***.***> wrote:  

Part of the readback block is to reduce all possible readable registers into a single flattened array. Since not all registers are necessarily going to to be readable by software, this is not necessarily going to simply be the accumulation of all enclosed registers in an array. In order index into this flattened array, the cumulative number of elements prior needs to be tracked. This is done via the n_regs variable.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[galaviel]

BTW another thought- since parameter values are not known at time of generation, part of the validation cannot be done. How do you feel about inserting SystemVerilog Assertions to do the validation at runtime in this case?

[galaviel]

One more thing, in PeakRDL-regblock\src\peakrdl_regblock\readback\generators.py, you use the term "$i{i}sz" because you do not know in advance the size of the array. I wonder why that is - isn't the entire register supposed to be SW readable or not, and thus its dimensions are known in advance? why there is a need to search and replace the actual array size after iterating through all the array dimensions?

[amykyta3]

During iteration of a regfile, or nested regfiles, it is not known how many of the elements will be readable, so therefore it is not known how much of an offset is required between blocks. This is why the placeholder is emitted temporarily and then back-filled later.

Regarding validation - I was initially planning on bypassing validation of various aspects of the design since they are not known. Inserting assertions is an interesting idea that might be useful in some areas.

Lastly, I would love to see what you have done so far. Generally on GitHub, if you fork a repository to your personal workspace and push your changes, they will be visible for me (and others) to review. It would definitely be interesting for me to see how you implemented this feature. Of course, the challenge is always going to be to make it robust in all the general cases which is up to me to do :laughing:

[galaviel]

Not sure I follow regarding the place holder - probably something missing in my understanding. I'm still studying the code. Part of it I understand analytically, and part of it is just trial and error.. (things fail - I get a Python exception and then I fix that area). Sort of an opportunistic debug mode.. I guess if you need to emit a statement for the enclosing element/component before (temporally) the child, then you must use a placeholder. My example is simple - one address map, one register file, so I have not seen this yet. Perhaps if I iterate the reg file a few times inside the address map I will see this issue.

Validation - yes, that's what I did temporarily right now.. bypassing validation only when the reset value / array dimensions are a ParameterRef. Nothing else should be disturbed. I think emitting some kind of runtime checking or assertions eventually cannot be avoided since you don't know what funny things the users will do .. (can be enabled optionally via subcommand flag..)

Regarding review, sure, actually I've already forked the project. Need to finish everything 100%, make sure it compiles, LINT is clean (BTW I thought about adding a Synopsys Spyglass LINT test, to those who have access to the tool), and functionally it's working (I've substituted the PeakRDL implementation for the original RTL we have). Probably this week.

Thanks for your support!

[galaviel]

Hi,

I've uploaded a sketch implementation that allows parametrization of: Field reset value Array size (1D only! didn't try MDA)

I'm now working on parametrized field width. hopefully it'sll be easier and quicker. I think the biggest mess I did was in 'readback/generators.py', the rest is a bunch of small changes/fixes.

forked projects: https://github.com/galaviel/PeakRDL-regblock-Parametrized https://github.com/galaviel/systemrdl-compiler-Parametrized

I didn't touch the command line tool though.

One thing I didn't get right is the selective selection of parameters to keep symbolic. Right now if --keep_params is provided all parameters seen in register array dimensions + field reset values will be kept symbolic, even ones the user wishes to resolve. I will fix this.

I'm now adding the missing trial implementation of field msb/lsb being kept parametrized. Here, I'll pass down the specific list of params from the cmdline to the actual code. Only params the user requested to be kept symbolic will be kept as such. Rest will be resolved.

[jimmyli-421]

Hi @galaviel, Thank you for providing this useful feature. However, I encountered some errors while using it. Here is my error log:

Traceback (most recent call last):
  File "/home/kehan/ENTER/envs/RDL_PAR/bin/peakrdl", line 33, in <module>
    sys.exit(load_entry_point('peakrdl==0.8.0', 'console_scripts', 'peakrdl')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl-0.8.0-py3.11.egg/peakrdl/main.py", line 165, in main
    options.subcommand.main(importers, options)
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl-0.8.0-py3.11.egg/peakrdl/subcommand.py", line 154, in main
    self.do_export(top, options)
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl_regblock-0.12.0-py3.11.egg/peakrdl_regblock/__peakrdl__.py", line 149, in do_export
    x.export(
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl_regblock-0.12.0-py3.11.egg/peakrdl_regblock/exporter.py", line 233, in export
    stream.dump(module_file_path)
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/Jinja2-3.1.2-py3.11.egg/jinja2/environment.py", line 1618, in dump
    fp.writelines(iterable)
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/Jinja2-3.1.2-py3.11.egg/jinja2/environment.py", line 1613, in <genexpr>
    iterable = (x.encode(encoding, errors) for x in self)  # type: ignore
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/Jinja2-3.1.2-py3.11.egg/jinja2/environment.py", line 1662, in __next__
    return self._next()  # type: ignore
           ^^^^^^^^^^^^
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/Jinja2-3.1.2-py3.11.egg/jinja2/environment.py", line 1354, in generate
    yield self.environment.handle_exception()
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/Jinja2-3.1.2-py3.11.egg/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl_regblock-0.12.0-py3.11.egg/peakrdl_regblock/module_tmpl.sv", line 157, in top-level template code
    {{readback.get_implementation()|indent}}
^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kehan/ENTER/envs/RDL_PAR/lib/python3.11/site-packages/peakrdl_regblock-0.12.0-py3.11.egg/peakrdl_regblock/readback/__init__.py", line 35, in get_implementation
    if array_size < 4:
       ^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'str' and 'int'

Do you have any ideas about this problem? Below is the simple RDL code I used to try this feature:

reg myReg #(longint unsigned SIZE = 6) {    
    field {
        sw=rw;
        hw=r;
        reset = SIZE;
    } data[SIZE];
};

addrmap tiny {
    myReg TEST;
};

Any help would be greatly appreciated. Thanks in advance, Jimmy.

[galaviel]

Hi,

( Jimmy - will take look now )

I've uploaded the missing feature of msbit/lsbit parametrization.

Repo links - same 2 as before, and in addition - https://github.com/galaviel/PeakRDL-Parametrized.git (had to change the cmdline tool itself to pass cmdline options to the compiler).

Commit msg below. Note tested only with the trivial case of single field, lsbit=0 msbit=some parameter name. And seems to work (ran RTL simulation).

Again provisory, needs to be better (re)structured.

commit msg:

Adding support for keeping parametrized, the ms/ls-bit of the field. (note: only ms-bit tried and works. Single field in register, ms/ls-bits are [PARAM:0]. nothing else tried).

Note: ms-bit expression must be comprised only of the single parameter name. No arithmatic or other operands currently supported. (easy enough to add them later I think..)

Also, Fixing a conceptual bug: Previously if --keep_params was provided on the command line, all parameters were kept unresovled and kept symbolic. However, we probably want to do this selectively only for the parameters specified in --keep_params. This understanding came to me late in the process. For symbolic register array size + reset value, I didn't do this; if --keep_params is found on the cmdline, all register array dimensions and field reset values which are parameters, are kept symbolic, regardless if they appear in -keep_params or not.

But, for the Field MSbit, I now added this support. Only Field MSbits which are parameters and specified in -keep_params will be kept symbolic.

So, need to pass 'options' (cmdline options) to the compiler, since the compiler/elabortor needs to selectively keep symbolic only those Parameters the use wishes to keep symbolic.

[galaviel]

Hi Jimmy,

Please try '[SIZE:0]' instead of just [SIZE] to define the field. For some reason the latter isn't working. Need to figure out why. Also, please pull again from all 3 repo's since keeping the ms/ls-bits of the field as a parameter was not pushed until today.

Let me know how goes. Lastly, might want to throw in a 2nd register, I noticed the generated RTL code might have trouble with only 1 register; says " assign readback_array[][SIZE:0] = " where the 'readback_array' index is empty. Not sure if this is something I did or just a bug in the original code.

[jimmyli-421]

Hi @galaviel,

Thank you for your reply!!! However, I am still encountering some errors. Can you please help me confirm if my installation process is correct? Here are the steps I followed:

1. I pulled the repository from https://github.com/galaviel/PeakRDL-Parametrized.
2. Then, I navigated to the directory by runningcd PeakRDL-Parametrized.
3. Finally, I ran python3 setup.py installto install the tool.

Please let me know if there is anything I missed or if there are any additional steps I need to take.

The following is the error message

$ peakrdl regblock tiny.rdl --keep-params SIZE -o ./
Traceback (most recent call last):
  File "/home/kehan/ENTER/envs/TRT/bin/peakrdl", line 33, in <module>
    sys.exit(load_entry_point('peakrdl==0.8.0', 'console_scripts', 'peakrdl')())
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl-0.8.0-py3.10.egg/peakrdl/main.py", line 109, in main
    subcommands += get_exporter_plugins(cfg)
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl-0.8.0-py3.10.egg/peakrdl/plugins/exporter.py", line 44, in get_exporter_plugins
    cls = ep.load()
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/importlib/metadata/__init__.py", line 171, in load
    module = import_module(match.group('module'))
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 992, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl_regblock-0.12.0-py3.10.egg/peakrdl_regblock/__init__.py", line 3, in <module>
    from .exporter import RegblockExporter
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl_regblock-0.12.0-py3.10.egg/peakrdl_regblock/exporter.py", line 8, in <module>
    from .field_logic import FieldLogic
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl_regblock-0.12.0-py3.10.egg/peakrdl_regblock/field_logic/__init__.py", line 7, in <module>
    from . import sw_onwrite
  File "/home/kehan/ENTER/envs/TRT/lib/python3.10/site-packages/peakrdl_regblock-0.12.0-py3.10.egg/peakrdl_regblock/field_logic/sw_onwrite.py", line 6, in <module>
    from _ast import Param
ImportError: cannot import name 'Param' from '_ast' (unknown location)

Thank you!

[galaviel]

Hi Jimmy,

I do believe you need to pull from all 3 repositories (the compiler, command line tool, and regblock exporter) since I've edited all three.

Here's my setup. There is an order which you need to clone the repo's which @amykyta3 has specified in a previous discussion (can't find it now ..).

anyway here's my setup:

# create python virtual env
python -m venv venv1

# active the virtual env
venv1\Scripts\activate

# add the python.exe under C:\py\venv1\Scripts\python.exe as Pydev selected interperter

# clone
# in order to install with -e (edit) you must clone locally all those repo's
git clone https://github.com/SystemRDL/PeakRDL
git clone https://github.com/SystemRDL/PeakRDL-regblock
git clone https://github.com/SystemRDL/systemrdl-compiler

# install from the cloned location
# order matters (you want to reverse the order of your pip installs, otherwise pip will fetch the published packages from pypi): 
pip install -e systemrdl-compiler
pip install -e PeakRDL-regblock
pip install -e PeakRDL

# create small script as in 
# https://github.com/orgs/SystemRDL/discussions/165 
# and debug it. Note - might not be required.

hope this works for you - if not let's try to understand together what are the issues,

Thanks, Gal.

[jimmyli-421]

Hi @galaviel,

I just wanted to let you know that I was able to successfully install your repository using the instructions you provided. Thank you for your help!

However, I encountered an issue when running the command peakrdl regblock after installing your three parameterized repositories. Specifically, I received an error message that included the following traceback:

raceback (most recent call last):
  File "/home/kehan/ENTER/envs/try3/bin/peakrdl", line 33, in <module>
    sys.exit(load_entry_point('peakrdl', 'console_scripts', 'peakrdl')())
  File "/home/kehan/RDL_PARAM/PeakRDL-Parametrized/src/peakrdl/main.py", line 109, in main
    subcommands += get_exporter_plugins(cfg)
  File "/home/kehan/RDL_PARAM/PeakRDL-Parametrized/src/peakrdl/plugins/exporter.py", line 44, in get_exporter_plugins
    cls = ep.load()
  File "/home/kehan/ENTER/envs/try3/lib/python3.10/importlib/metadata/__init__.py", line 171, in load
    module = import_module(match.group('module'))
  File "/home/kehan/ENTER/envs/try3/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 992, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/kehan/RDL_PARAM/PeakRDL-regblock-Parametrized/src/peakrdl_regblock/__init__.py", line 3, in <module>
    from .exporter import RegblockExporter
  File "/home/kehan/RDL_PARAM/PeakRDL-regblock-Parametrized/src/peakrdl_regblock/exporter.py", line 8, in <module>
    from .field_logic import FieldLogic
  File "/home/kehan/RDL_PARAM/PeakRDL-regblock-Parametrized/src/peakrdl_regblock/field_logic/__init__.py", line 7, in <module>
    from . import sw_onwrite
  File "/home/kehan/RDL_PARAM/PeakRDL-regblock-Parametrized/src/peakrdl_regblock/field_logic/sw_onwrite.py", line 6, in <module>
    from _ast import Param
ImportError: cannot import name 'Param' from '_ast' (unknown location)

To resolve this issue, I manually commented out line 6 in the "sw_onwrite.py" file, which is located here. Maybe you can check if this line is necessary or not.

Once again, thank you for your help and for sharing your work with the community.

Sincerely, Jimmy.

[jimmyli-421]

Hi @galaviel,

I have another small question.

I was looking at the generated RTL module interface and noticed the following code snippet:

module tiny
#(
    longint unsigned BW1 ,      
    longint unsigned SIZE       
)
(
    input wire clk,
    input wire rst,
    apb3_intf.slave s_apb,
    tiny_if.out hwif_out
);

My question is regarding the longint unsigned SIZE parameter. It does not seem to be assigned to any specific value (longint unsigned SIZE = 8). Is this correct or should it be assigned a value?

Thanks.

[galaviel]

Hi Jimmy,

Regarding the redundant import, you are right. I've removed this line & pushed to github. For some reason it didn't cause any issues for me, perhaps our installs are still different and mine didn't exhibit this issue. Regardless, that line is redundant and is no longer there. Thanks for noticing!

Regarding the unassigned parameter, if I understand correctly what you were asking, then I would say that's the whole point. (keeping it unassigned). The user that will instantiate this 'regblock' will set the value for this parameter. However the regblock's module definition does not specify the default value. (which is allowed in Verilog, you don't have to assign default values to module parameters, if I'm mistaken). It is an elaboration error if eventually that parameter is not resolved to any actual value, again - in this case the value should be set by the verilog module instantiating the regblock.

Hope this is what you meant, if not do let me know,

Thanks, Gal.

[jimmyli-421]

Hi @galaviel,

Thnaks for your response! I understand and agree with your thoughts.

I have a new situation that I'm not sure if you have considered before. In this case, I need to instantiate two of myReg, named REG1 and REG2, and they both need to be reset to VALUE1 and VALUE2 respectively. However, it seems that we can only assign one value to the VALUE parameter in the generated RTL code. I think a possible solution is to define another reg component, but this would make the code more verbose, and these two regs would be too repetitive. Do you have any good ideas or solutions for this situation?

reg myReg #(longint unsigned VALUE = 6) {
    field {
        sw=rw;
        hw=r;
        reset = VALUE;
    } data[8];
};

addrmap tiny {
    myReg REG1;//needs reset to VALUE1
    myReg REG2;//needs reset to VALUE2
};

I hope that my question was clear, but please let me know if there's anything that needs clarification.

Thank you, Jimmy

[galaviel]

Hi Jimmy,

Sorry for my late reply .. was lost in the flood of emails.

I see your point. I'll work on fixing that.

keep you posted..