-
Notifications
You must be signed in to change notification settings - Fork 58
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
Decide on the specific simulation and synthesis behavior for out-of-bounds Array access #7
Comments
I would prefer to have a well defined default. The last or first element. That's what mögen dies now. It's also what you want in practice. I suspect x won't synthesize and isn't something you want to face. And an empty array should be an error IMO. |
Of course it will, |
@jordens Are you opposed to making out-of-bounds access a hard error in simulation? Why? I understand having a well-defined default for synthesis instead of |
But x doesn't mean "Impossible". If the synthesizer encounters it, afaik it will bail. X doesn't unify with any (valid) downstream logic. In if you out of bounds selectors, the only possible (synthesizable, in practice on fpga) end solution will be a default. |
@jordens This is not true. IEEE 1364.1-2002 section 5.5 explicitly allows using X on RHS of assignment to specify a don't care value, which is exactly what I want here: Of course, the end result will usually be that the synthesized logic will select some multiplexer input as an output, but the whole point here is to ease the pressure on the synthesizer and let it not use any extra |
(And moreover, e.g. Yosys will happily introduce X values internally during synthesis when reorganizing logic. So of course it has to handle X correctly. Everything that uses the Synopsys frontend also should handle that correctly; Synopsys handles X and Z far better than Yosys, including permitting Z intra-chip...) |
Ok. In general the offerings of the verilog spec should be checked against practicability. Does this also hold for e.g. vivado? I'd imagine that allowing x will taint all downstream logic and in turn prevent timing analysis (for example). |
This is what I believe will happen, yes.
No X propagation. Just a hard error (exception, or maybe something equivalent but nicer) if you ever index an array out of bounds. When is this ever not a coding error? Especially given that Migen docs unambiguously require the array index to always point within bounds. |
That sounds problematic. It would lead to valid and desirable (for synthesis) code that you can't simulate, right? |
The other way around. Using X on RHS is communicating to the synthesizer that it can substitute whatever it wants with that expression because it will never happen because of some invariant in your design that you know holds true but the synthesizer may not be able to infer on its own. So these are two sides of the same coin. The invariant is that OOB access will never happen. In simulation, if this invariant is violated, we can detect it directly because it's easy, and indicate it as a programming bug. In synthesis, we assume that the invariant is never violated, and communicate this to the synthesizer. So long as the invariant is not actually violated, which is the contract Migen already requires you to uphold, neither the simulation behavior (which is an exception) nor the synthesis behavior (which is an arbitrary, possibly nonsensical value being assigned), actually ever happen. There is no simulation-synthesis mismatch because the out of bounds case never happens neither in simulation nor in synthesis, and there is nothing that may or may not 'match'. |
Are you sure that vivado handles x correctly (it can do whatever it wants and uses it to simplify logic)? |
After discussion with @sbourdeauducq I agree with this. The default will be the same as in Migen, with using X as an opt-in possibility. |
Migen left this undefined so we have wide latitude here. IMO the following should happen:
x
s, to let the synthesizer discard useless logic. Migen's output causes waste in inferred multiplexers, and Migen's output already causes way too many multiplexers to be inferred.The text was updated successfully, but these errors were encountered: