Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order of operations of addr calculations (bit shift must occur before addition) #1082

Closed
Tracked by #1072
nathanielnrn opened this issue Jul 6, 2022 · 4 comments · Fixed by #1091
Closed
Tracked by #1072
Labels
C: FPGA Changes for the FPGA backend

Comments

@nathanielnrn
Copy link
Contributor

No description provided.

@nathanielnrn
Copy link
Contributor Author

nathanielnrn commented Jul 6, 2022

Another one I'm not sure about @rachitnigam. The relevant generation occurs here. Could you point me in the direction of parentheses logic//forcing parentheses?

@sampsyo
Copy link
Contributor

sampsyo commented Jul 7, 2022

This looks to me like a bug in the VAST library! In fact, it looks extremely similar to a previous bug already fixed by @rachitnigam in vegaluisjose/vast#6. VAST uses a Verilog operator precedence function defined here:
https://github.com/vegaluisjose/vast/blob/ae19379034a2d520cd572c0a1cac2f735f8269a6/src/subset/pretty_print.rs#L41-L42

…and it doesn't seem to explicitly consider shifts. Seems like this needs a tweak to make the parens appear in the right place.

A possible workaround for now would be to assign the shift expression to a temporary variable and then use that (which would obviate the need for parentheses).

@rachitnigam
Copy link
Contributor

@nathanielnrn can you open an issue in the vast library saying what the expected behavior vs. actual behavior is and tag me? I can fix it once I have a test. An easy way to figure this out would be to clone the vast repo and add a new test here: https://github.com/vegaluisjose/vast/blob/ae19379034a2d520cd572c0a1cac2f735f8269a6/tests/v05.rs

The test would let you specify what the output verilog should look like

@rachitnigam rachitnigam added the C: FPGA Changes for the FPGA backend label Jul 8, 2022
@nathanielnrn
Copy link
Contributor Author

Addressed as an issue in the vast repository as well.

@nathanielnrn nathanielnrn linked a pull request Jul 11, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: FPGA Changes for the FPGA backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants