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
Partly out of range part-select gives wrong runtime result #1027
Comments
Original Redmine Comment This is related to #�. The problem is fixing it requires significant runtime code which will may massively slow simulations. More thought is needed on how to reduce this burden. |
Original Redmine Comment Did you have something in progress on this one (as mentioned in the linked ticket)? we have multiple designs impacted by this at the moment so if not I'll likely take a look soon. |
Original Redmine Comment I don't. Basically right now it looks at the msb/lsb range and makes a Bound check, which either turns on or turns off the indexing. It needs to become a mask. (and perhaps leave the Bound conditional as all-on will be the common case.) Thanks |
Original Redmine Comment So I started with this, and it's a complete mess -- Any advice how to make this less terrible would be appreciated. A couple of high level concerns:
I've uploaded a very early draft here, it still has plenty of failing tests and indentation/comments is a mess. Some early feedback would be very appreciated, just on a high-level whether this approach seems about right? phb/verilator-dev@e530ee4#diff-6c80b812607617c162e46fd56e6579c8R951 Thanks |
Original Redmine Comment
Changing all of this will have huge runtime penalties unless we can eliminate most of this generated code by the end. I wonder if it would be better to have V3Width protect all selects with some new Ast type (similar to AstCondBound), then have later stages tune as needed, and teach V3Const how to simplify them away. V3Unknown would do the final cleanup searchng for that new node type and add the AstIf's.
|
Original Redmine Comment Realized I broke one of the tests I have been caring about in the last upload, again relating to the question on how to do sign extensions and Adds/Subtracts properly. Ie, I get results like this:
(for t_select_plus) |
Original Redmine Comment Wilson Snyder wrote:
Makes sense, will do.
Agreed, the generated code looks awful at the moment. I'm trying to think of specific optimizations that could be done to optimize away this new node type. Are you thinking about checking the range of the input variable (or constant) and remove it in the cases where it's always in range. Any others?
|
Original Redmine Comment
Yes. Another common pattern will be var[var2] of width 1. If var is a power-of-two, and var-2 is log2 that size, then there's no way to go out of range. Practically I think the thing to do is take one of your large designs (plus those in test_regress), run with the old version, run with the new version, and do a tree diff with var names removed, to see what new spots got ugly and to see if they can be optimized away. Also if after all of that we still have too many extra instructions we may want a switch to disable this. |
Original Redmine Comment I'll keep whacking away and see if I get somewhere. More about adding various math operators to the Ast after V3Width. Is the contract that the two values passed in are the same type? Or that dtype lhsp is large enough to store the result, but length of rhsp does not really matter? |
Author Name: Johan Bjork
Original Redmine Issue: 1027 from https://www.veripool.org
Original Date: 2016-01-24
According to 1800-2012:
"A part-select that addresses a range of bits that are completely out of the address bounds of the vector, packed array, packed structure, parameter or concatenation, or a part-select that is x or z shall yield the value x when read and shall have no effect on the data stored when written. Part-selects that are partially out of range shall, when read, return x for the bits that are out of range and shall, when written, only affect the bits that are in range." (11.5.1)
Code in V3Unknown.cpp seems to handle the first case, but not partly out of range assignments
Basic hardcoded example (Silly, but still exhibits the issue, more relevant when using variables)
The text was updated successfully, but these errors were encountered: