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

Support division of negative numbers? #478

Closed
pepijndevos opened this issue Aug 17, 2020 · 5 comments
Closed

Support division of negative numbers? #478

pepijndevos opened this issue Aug 17, 2020 · 5 comments

Comments

@pepijndevos
Copy link
Contributor

It appears that Yosys now supports $divfloor as of YosysHQ/yosys@edd8ff2

In ast.py it is mentioned that the different semantics mean it's not supported:
https://github.com/nmigen/nmigen/blob/e46118dac0df315694b0fc6b9367d285a8fc12dd/nmigen/hdl/ast.py#L173-L179

I'm not sure what division synthesizes to in Yosys in general, or if it supports the general case at all, but it seems that on the nMigen side we can now emit $divfloor and fully support division with correct semantics. I'd be happy to put together a PR for this at some point.

@whitequark
Copy link
Member

Reasonable. I think we might want to wait until there's a Yosys release, because we can't shim over the lack of $divfloor in older Yosys in back.verilog, like we do for other Yosys-related variability.

@pepijndevos
Copy link
Contributor Author

The verilog backend in that commit (which I do not claim to fully understand) does seem to "shim" over the lack of flooring division in Verilog, so I think it might technically be possible to work around it, but I agree waiting for a Yosys release is easier. Bit unfortunate that Yosys releases are not exactly frequent.

@whitequark
Copy link
Member

I think it might technically be possible to work around it

I guess you could, when targeting older Yosys, always provide a $divfloor cell as a part of nmigen.back.verilog, and then immediately flattening it, so that it never actually appears in output Verilog. But it's not clear that the effort is justified.

@pepijndevos
Copy link
Contributor Author

Probably not...

@whitequark
Copy link
Member

This is implemented as of b452e0e (issue #621).

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

No branches or pull requests

2 participants