Updates and improvements to SDF parser. - #51
Conversation
). This commit squashes all the changes made in PR chipsalliance#51. The summary of important changes made by PR chipsalliance#51 is as follows: * Removed parser internal helper collections. The updated parser now returns the parser structure as a return value of grammar rule expansions. This change simplifies the code and would enable future refactoring to the visitor pattern (for greater flexibility of the parser). * Changed internal structure of parsed data to better represent input data 1:1. The new structure keeps any duplicities if they existed in the input SDF and it preservers order of SDF timing records. These changes are to better correlate emitted parsed SDF to its original input. * Added support for more SDF constructs (e.g. `RECREM`, `PERIOD`, `RETAIN`), incl. SDF comments. The comments are handled by the lexer as an ignored token and hence does not presently propagate to the parser. * Fixed semantics of delval_list to correspond to SDF 3.0 Std. * Added unit tests of individual SDF syntax elements. The unit test is however far from complete. * Added unit tests of complete SDF parsing and writing out the parsed data back to SDF. This is much like the original `parse_all_test.py` except that `test_parse_and_write.py` operates on SDF strings and not files. * Added few simple utilities, e.g. to convert SDF to JSON or to reformat SDF. * This update makes the `parse_all_test.py` test suite fails as some of the golden files no longer correspond to how the parsed data looks like now, when emitted into a file. To make the test pass again, new golden file set is to be generated and reviewed for expected contents. The rest of commit message consolidates messages from individual commits that were squashed during the rebase process but were worth to keep. * Changes to expressions: > The *conditional port expression* (`cond_expr`) is part of the `COND` > conditional path delay. Past implementation did not handle bracketing. > New implementation split the grammar between binary expressions and > unary expressions. > Presently we do care only for the expression string and not about > (binary) operator precedence. Hence we do not build the (precedence) > binary tree for the expression. The present implementation is likely > to have better performance and if the operator precedence determination > were needed, I would suggest to have a separate, on-demand parser for > it. > Along with the parser change, minor changes/fixes were made to the lexer. > Namely the `ARITHMETIC` token was broken into individual symbols, > `PLUS`, `MINUS` and `TIMES`. This was due to `PLUS` and `MINUS` acting > as both unary and binary operators. > `SCALARCONSTANT` was fixed not to match strings `0'b1` and `0'b0`, which > are not valid constants. * Fixed `real_triple` to allow single numeric value. > As per SDF Std., `rvalue` (which is equivalent to the `real_triple` parser > rule in sdfyacc.py) can be either a triple or a single value. The latter > was missing in the grammar and fixed by this commit. * Added `PERIOD` and `NOCHANGE` timing checks. > The two timing checks were missing in grammar rules and no lexical > tokens were defined for them. Also added some basic unit tests. * Reduced usage of helper collections in parser. > The legacy parser code have been using global collections to aggregate > various parsed constructs. This coding style is very fragile for refactoring > and there is also no reason why to use it. Instead, one can aggregate the > collections within return values of grammar rules. > This commit should fix the above coding pattern for lists of DELAY, > TIMINGCHECK and TIMINGENV collections. * Added `RETAIN` and `RECREM`. Added an ignored comment token to lexer. Fixed `delval_list`. > Fixed/changed `delval_list` grammar rule to follow SDF standard (3.0). > Now one, two, three, six and twelve delays can be used. They are then > expanded into a map of twelve transitions (as per Std.). In legacy > code, the delay value were treated as slow/nominal/fast delays, which > was obviously away from SDF standard. > The retain delay is put under its parent `iopath`. The retain delval > list goes under the `retain_paths` dictionary key. This key is populated only > when the `IOPATH` has an associated `RETAIN`; otherwise the `retain_paths` > key will not exist. The existince of `RETAIN` can be tested through a bool > key `has_retain` that will be populated for every `iopath` type timing > arc/record structure. * Uniquification of names of timing annotations. > When adding collected timing annotations under the cell construct, > it may happen that there will be two or more annotations with the > same name. For example, two conditional IOPATH delays (with different > conditions) for the same from-to pair of pins. The way how the > derived timing annotations are named and indexed under the cell > will yield name collision. > This commit adds code to detect such collisions and append a sequential > number to uniquify every colliding occurrence. For example:: (CELL (CELLTYPE "AND2") (INSTANCE top.b.d) (DELAY (ABSOLUTE (COND !en (IOPATH a y (1:2:3)(1:2:3))) (COND en (IOPATH a y (4:5:6)(4:5:6))) ) ) ) > The first annotation will be named `iopath_a_y` and the second > `iopath_a_y_1`. * Timing checks grammar rules now return scalar. * Changed delval_list result to remain a list. > This is to be able to perform original stability tests. Otherwise it would be > impossible to identify how many delval entries there were in the input SDF. > Also removed sorting delays for a cell as there is no reason to do so. * Changed dict keys holding removal and recovery times. > Changed `removal` to `hold` and `recovery` to `setup` for the `recrem` > timing check to make it easier for printing out (as there will be the > same keys for `recrem` as for `setuphold` and `nochange` arcs). * Temporary util `utils/sdfwriter.py` using legacy `sdfparse.emit()`. > Temporarily added a new utility that works like `utils/reformat.py` but uses > the legacy SDF formatting API. The utility is to prep for updates in `cell` > and `instance` structure reorganization in the parser. Present tree-like > structure cannot handle well name duplicities in cells or instances. Cell > name duplicities now yields partial loss of sequential order in the parsed > structure (when compared to the input file). Instance name duplicities > (assuming the same cell name) will lead to loss/overwriting of certain > timing data. While cell & instance duplicities should not generally occur > in a semantically correct SDF input, their resolution shall be deferred > after parsing. > The above limitations will be addressed in future commits. The new utility > will be used for testing to ensure the future `sdfparse.emit()` will remain > compatible with the legacy one as much as possible. * Changed parsed structure of scalar delay values. > Newly represent a scalar delval (e.g. `(1.1)` or `()`) by using a dictionary > with a single `all` key rather than with the `min`, `avg` and `max` key triplet > with the same value. This helps to indicate the parsed value was indeed a scalar > and not a triplet. * Changed parsed structure to list of cells. > Changed the `cells` key under the timings structure to be a list rather > than a dictionary. This will keep order of cell records as they were in > the input SDF file.
|
Hi Karol, |
|
@brabect1 - You just push over the previous branch and it will update the pull request -- seems like you figured that out? |
|
there seems to be a number of python errors like e.g.: |
|
Hi guys, I will need more guidance on how to fix these CI workflows. Say a make a change that is assumed to fix a problem. I guess I have to push into master branch. How do I run the CI tasks then to see if the fix worked? |
|
@brabect1 opening PR should be enough to trigger the CI. |
|
Hi, can someone trigger CI to see if there are more things to fix? thanks |
CI was triggered and finished, but it is still red - mostly in package tests:
|
|
Hi Karol, I did one more fix. Could you please trigger CI again? I am not able to reproduce the issue locally, which is why this trial and error approach ... |
|
Ok, looks good. Now the interesting part. So I guess someone from the team should review, if the new output is ok. You can either generate the new reference set yourself, or I can make it a part of the pull request (replacing the old reference set). Which way you prefer? |
|
Hi guys, as no one responded to my last post I made updates to the golden set files to match what the parser now generates. Can you trigger CI so we see there are no other issues? You can then review the changes to the golden set. Thanks. |
| (TIMINGCHECK | ||
| (HOLD CLK (posedge CLK) (0.507::0.411)) | ||
| (HOLD CLK (posedge CLK) (0.728::0.538)) | ||
| (HOLD CLK (posedge CLK) (0.579::0.46)) |
There was a problem hiding this comment.
why do we have 4 entries for the same timing? Shouldn't this be handled by speed corners?
There was a problem hiding this comment.
The duplicate arcs are due to the fact they are in the input clb.sdf file. The original parser code removed these duplicate entries so that it kept the last record. A parser shall not do that as it would be then losing information it provides to downstream SW layers. Think that you would like to do pretty printing or write a syntax aware editor; it would not be possible if the parser is removing syntax elements.
So if you compare the output SDF to clb.sdf input, you would notice that now they are the same. The only difference is that sdfparse.emit() sorts the parse elements so they appear reordered in the output file. I did not fix emit() to not do sorting. Instead, I created a new output sdfwrite.print_sdf() that writes out elements in the order of the input. To see the difference, compare the outputs of utils/sdfwriter.py and utils/reformat.py. For example:
python3 utils/sdfwriter.py --stdout sdf_timing/tests/data/clb.sdf
python3 utils/reformat.py --stdout sdf_timing/tests/data/clb.sdf
| (DELAY | ||
| (ABSOLUTE | ||
| (COND (A == 1'b0 && B == 1'b0 && C == 1'b0) | ||
| (COND (B == 1'b0 && C == 1'b0 && D == 1'b0) |
There was a problem hiding this comment.
any idea why the conditions here change?
There was a problem hiding this comment.
The original output file was wrong. Compare to the input fixpoint.sdf. I do not remember if this was a problem in the parser or in the sdfwrite.emit(). If you compare the new golden output file with the test input file, they should match now.
| (PATHCONSTRAINT I2.H01 I1.N01 (989.0:1269.0:1269.0) (989.0:1269.0:1269.0)) | ||
| (PATHCONSTRAINT I2.H01 I3.N01 (904.0:1087.0:1087.0) (904.0:1087.0:1087.0)) | ||
| (PATHCONSTRAINT I1.N01 I2.H01 (989.0:1269.0:1269.0) (989.0:1269.0:1269.0)) | ||
| (PATHCONSTRAINT I3.N01 I2.H01 (904.0:1087.0:1087.0) (904.0:1087.0:1087.0)) |
There was a problem hiding this comment.
Hmm, that is not right. This is a problem in sdfwrite.emit(). If I recall right, the original parser did incorrectly swap source and destination ports in few cases; guess that this is one of them. I fixed that in the parser but not in emit() (either because I did not notice or because it wasn't easily possible). After all, I think that emit() should be discontinued in favor of print_sdf(). To see difference between the two, compare outputs of utils/sdfwriter.py and utils/reformat.py on the input file in question.
| #!/usr/bin/env python3 | ||
| # coding: utf-8 | ||
|
|
||
| # Copyright 2022 Tomas Brabec |
There was a problem hiding this comment.
can we stay with F4PGA Authors (as it is in all the other files)?
There was a problem hiding this comment.
I guess so as long as you eventually merge in the pull request; there is nothing special about the test code that I would regret losing copyright to. You may then add me to AUTHORS if you like and consider this pull request a significant contribution; that's really up to you.
So once we resolve all the other comments, I will change the notice.
|
The PR looks good in general, I left a few comments |
4 participants
This pull requests makes quite a few changes to the original code. The list of major updates:
RECREM,PERIOD,RETAIN), incl. SDF comments. The comments are handled by the lexer as an ignored token and hence does not presently propagate to the parser.delval_listto correspond to SDF 3.0 Std.parse_all_test.pyexcept thattest_parse_and_write.pyoperates on SDF strings and not files.This update breaks some compatibility with the original code. This makes the
parse_all_test.pytest suite fails as someof the golden files no longer correspond to how the parsed data looks like now, when emitted into a file. To make the test
pass again, new golden file set is to be generated and reviewed for expected contents. This is something I ceased to
do as my expectations of the output data may be different than that of the original authors.