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

Renamed translate_off to ` ifndef SYNTHESIS `` #210

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

sifferman
Copy link
Contributor

@sifferman sifferman commented Jan 21, 2024

Hello!

According to the IEEE 1364.1-2005 spec 6.3.2, the use of translate_off/translate_on is discouraged as opposed to `ifndef SYNTHESIS. Plus, some newer SystemVerilog frontends such as sv2v, Verible, and PySlint do not have any extra AST support for translate_off, so they may get confused by unsupported unsynthesizable constructs.

Therefore, I changed 38 occurrences of translate_off to `ifndef SYNTHESIS. However, I kept 4 deprecated modules with translate_off because I wasn't sure whether they should match the non-deprecated modules' style:

Also, I would be happy to hear another perspective if you feel that translate_off is in fact the better option.

Thanks!

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sifferman thanks a lot for your contribution, and sorry for my late response. To make this complete, could you also replace the pragmas in stream_fifo_optimal_wrap.sv?

I'm open to updating this in the deprecated modules but don't think it's strictly necessary.

@niwis niwis mentioned this pull request Mar 6, 2024
@sifferman
Copy link
Contributor Author

@niwis Thanks for the response, and good catch! I corrected stream_fifo_optimal_wrap.sv. I also added the changes for the deprecated modules.

I can squash if needed.

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! LGTM

@niwis niwis merged commit a88e217 into pulp-platform:master Mar 6, 2024
2 checks passed
@sifferman
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants