EBLIF cell parameter interpretation #1663

mkurc-ant · 2021-02-12T14:33:52Z

A proposed solution to #1662

Updated EBLIF cell parameter specification
Added EBLIF parameter value validation on reading
Added binary prefix to relevant parameters in post-routing Verilog netlist writing.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

litghost · 2021-02-12T16:20:11Z

doc/src/vpr/file_formats.rst

+ * **binary word**
+    Binary words are specified using strings of characters ``0`` and ``1``. No other characters are allowed. Number of characters denotes the word length.
+
+ * **real number**


What is .param x 2? A real number or error?

Its an error. For an integer 2 you need 10, for a real 2 you need 2.0.

I think this needs to make clear that 2 is illegal, because it is not a binary word (has a 2) and is not a string (no quotes) and not a real number (no .).

Added clearer explanation of illegal formats

vaughnbetz

Looks good. A few wording / variable name tweaks suggested, and one bug to fix (digit 9 would not be considered part of a real right now).

vaughnbetz · 2021-02-12T16:35:22Z

doc/src/vpr/file_formats.rst

+    .param multiplier 0.50
+    .param power 001101
+
+Would set the parameters ``feedback``, ``multipleir`` and ``power``  of the above ``pll`` ``.subckt`` to ``"internal"``, ``0.50`` and ``001101`` respectively.


typo: multipleir -> multiplier

vaughnbetz · 2021-02-12T16:36:19Z

doc/src/vpr/file_formats.rst


 ``.param`` statements propagate to ``<parameter>`` elements in the packed netlist.

+Paramerer values propagate also to post-route Verilog netlist. Strings and real numbers are passed directly while binary words are prepended with the ``<N>'b`` prefix where ``N`` denotes a binary word length.


slight wording change:
"to the post-route Verilog netlist, if it is generated."

vaughnbetz · 2021-02-12T16:38:23Z

vpr/src/base/read_blif.cpp

+}
+
+bool is_real_param(const std::string& param) {
+    const std::string chars = "012345678.";


Missing 9 --> need to add it to "chars".
Suggest using a more descriptive name than chars. real_chars would be better.

Right... stupid mistake. Fixed.

I gave it more thought and made the function use a regular expression.

litghost · 2021-02-18T17:38:34Z

vtr_flow/benchmarks/tests/test_eblif.eblif

-.param test_latch_param "test_latch_param_value"
+.param test_latch_param "test_latch_param_str_value"
+.param test_latch_param_bin 00110101
+.param test_latch_param_num 2.0


You need negative tests here!

Also I recommend reviewing the failure output to make sure it is possible to understand the problem from the error message. Concrete things I'd look for:

Is the location of the error made clear (e.g. file and line number)

Is there an explanation of why the input was wrong?

For example, missing quotes

A bare integer with no period

I believe that VPR at this point doesn't support test cases that are meant to fail (however https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/scripts/python_libs/vtr/util.py#L73-L75 allows that but this parameter is not used anywhere in the test framework).
Anyway, I tested the error output and modified it to provide more useful information.

litghost

Comments in

litghost · 2021-02-18T17:41:21Z

doc/src/vpr/file_formats.rst

+Parameters can have one of the three available types. Type is inferred from the format in which a parameter is provided.
+
+ * **string**
+    Whenever a parameter value is quoted it is considered to be a string.


Need to clarify that escaping rules (if any). For example what is .param "\""? Is that an error? What is it's behavior?

It seems like the external parser is not allowing escaped quote chars. In such case, VPR will throw syntax error there before the flow even gets to the logic responsible for distinguishing between 3 available formats of .param.
I modified documentation to highlight this.

vaughnbetz · 2021-07-30T16:28:57Z

@mkurc-ant : just prodding on this one. It would be good to address the review comments and get this one in soon.

mkurc-ant · 2021-08-02T07:10:54Z

@vaughnbetz Right. I'll have some time to look into it in the second part of this week.

lpawelcz · 2022-01-21T14:13:16Z

Hi, I updated this PR and responded to review comments because those changes will be useful for incoming PR that will deal with #1946.

@vaughnbetz can I ask you to take a look at my updates?

mkurc-ant · 2022-03-16T12:18:08Z

@vaughnbetz Could you please take look at this PR?

Signed-off-by: Maciej Kurc <[email protected]>

…ession. Signed-off-by: Maciej Kurc <[email protected]>

Signed-off-by: Pawel Czarnecki <[email protected]>

…in string params Signed-off-by: Pawel Czarnecki <[email protected]>

vaughnbetz

LGTM, thanks.

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Feb 12, 2021

mkurc-ant linked an issue Feb 12, 2021 that may be closed by this pull request

Cell parameters in post P&R verilog #1662

Closed

mkurc-ant requested review from vaughnbetz and litghost February 12, 2021 15:09

litghost reviewed Feb 12, 2021

View reviewed changes

vaughnbetz requested changes Feb 12, 2021

View reviewed changes

mkurc-ant force-pushed the eblif_params branch 2 times, most recently from 547c32b to 3480a0a Compare February 15, 2021 13:02

sfkhalid mentioned this pull request Feb 17, 2021

vtr nightly runs not completing #1666

Closed

mkurc-ant force-pushed the eblif_params branch from e01ba31 to e2432ab Compare February 17, 2021 14:57

mkurc-ant requested review from vaughnbetz and litghost February 18, 2021 11:44

litghost reviewed Feb 18, 2021

View reviewed changes

litghost requested changes Feb 18, 2021

View reviewed changes

litghost reviewed Feb 18, 2021

View reviewed changes

lpawelcz force-pushed the eblif_params branch 2 times, most recently from 84d502e to faebf9d Compare January 21, 2022 13:58

lpawelcz mentioned this pull request Jan 25, 2022

Netlist Writer: write post synthesis netlist that can be simulated #1957

Merged

7 tasks

mkurc-ant changed the title ~~[WIP] EBLIF cell parameter interpretation~~ EBLIF cell parameter interpretation Mar 16, 2022

mkurc-ant requested a review from sfkhalid March 22, 2022 10:52

mkurc-ant and others added 6 commits March 23, 2022 10:16

Added specification on how EBLIF cell parameters should be interpreted.

27c7402

Signed-off-by: Maciej Kurc <[email protected]>

Added EBLIF parameter validation and fixed post-route verilog output

c28468f

Signed-off-by: Maciej Kurc <[email protected]>

Updated documentation and code according to the review comments

9d31804

Signed-off-by: Maciej Kurc <[email protected]>

Updated the strong_eblif_vpr test

5de5dbb

Signed-off-by: Maciej Kurc <[email protected]>

Changed EBLIF real number value validation code to use a regular expr…

9752d01

…ession. Signed-off-by: Maciej Kurc <[email protected]>

doc: vpr: file_formats: warn about unsupported parameters formats

71fc191

Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz added 2 commits March 23, 2022 10:16

vpr: base: blif parse: descriptive error message for params

996f3aa

Signed-off-by: Pawel Czarnecki <[email protected]>

docs: vpr: file formats: blif: explain no support for escaping chars …

1cbc9b9

…in string params Signed-off-by: Pawel Czarnecki <[email protected]>

mkurc-ant force-pushed the eblif_params branch from faebf9d to 1cbc9b9 Compare March 23, 2022 09:16

vaughnbetz approved these changes Mar 24, 2022

View reviewed changes

vaughnbetz merged commit 980b450 into verilog-to-routing:master Mar 24, 2022

lpawelcz mentioned this pull request Jun 30, 2022

VPR: validate EBLIF parameters and fix them in verilog netlist writer lnis-uofu/OpenFPGA#697

Merged

6 tasks


		``.param`` statements propagate to ``<parameter>`` elements in the packed netlist.

		Paramerer values propagate also to post-route Verilog netlist. Strings and real numbers are passed directly while binary words are prepended with the ``<N>'b`` prefix where ``N`` denotes a binary word length.

EBLIF cell parameter interpretation #1663

EBLIF cell parameter interpretation #1663

Uh oh!

Conversation

mkurc-ant commented Feb 12, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Jul 30, 2021

Uh oh!

mkurc-ant commented Aug 2, 2021

Uh oh!

lpawelcz commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkurc-ant commented Mar 16, 2022

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpawelcz commented Jan 21, 2022 •

edited

Loading