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

New WIDTH warnings on genvars #1487

Closed
veripoolbot opened this issue Aug 4, 2019 · 7 comments
Closed

New WIDTH warnings on genvars #1487

veripoolbot opened this issue Aug 4, 2019 · 7 comments
Assignees

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Aug 4, 2019


Author Name: Wilson Snyder (@wsnyder)
Original Redmine Issue: 1487 from https://www.veripool.org

Original Assignee: Todd Strader (@toddstrader)


The following in git (unreleased) creates a warning where previously it did not, due to fixing #�. While technically this is a WIDTH warning it is probably not helpful to users (unless the i has a MSB set that isn't representable in r).

 reg [4:0] r;
 for (genvar i=0; i < 15; ++i)
   initial begin
      r = i;
   end
@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 9, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-09T10:10:02Z


Added two new tests in trunk, t_lint_width_genfor_bad which correctly fails
(though when this bug is fixed might have some minor output changes), and
t_lint_width_genfor which shouldn't fail but does, so is currently marked
unsupported.

Tracked the version that broke it. Was expecting it would be my fix for
#� relating to widthing parameters, but was the V3Number constant
changes towards #� in commit b045111.
The previous commit 654571b seems ok.

Looking at V3Simulate what seems to be going on is the old code kept the
V3Number's widthMin when making a data type, but the new code assigns the
data type based on the output data type which has a width but no width min.

The following hack makes things better, but doesn't fix integers (maybe fix
for #� conflicts?), and is certainly too ugly to be an accepted fix:

      void newOutValue(AstNode* nodep, const AstConst* constr) {
          newOutValue(nodep)->num().opAssign(constr->num());
 +        // Keep widthMin from the assigned number
 +        //FIXME hacked code from initWithNumber()
 +        //FIXME really should get right dtype from beginning
 +        AstConst* setnodep = fetchOutConstNull(nodep);
 +        const V3Number& m_num = setnodep->num();
 +        if (!m_num.isDouble() && !m_num.isString()) {
 +            //FIXME always using widthMin
 +            //FIXME do we properly set setnodep->num().sized?
 +            setnodep->dtypeSetLogicUnsized(m_num.width(), m_num.widthMin(),
 +                                           AstNumeric::fromBool(m_num.isSigned()));
 +        }
      }

I suspect #� might break again with this stuff fixed, IMO this bug is
more important than #�, so ok to fix this and reopen #�.
For #� might need to rework the widthUnsized()
tracking to properly know what is unsized versus what we're tracking a
widthMin for just to avoid spurious WIDTH messages. Either a new flag
tracked in the structures (started on this), or a hack where a negative
widthMin value means really unsized with given widthMin.

I also noted that the debug messages in V3Simulate no longer print the
value being assigned but always zero making it hard to debug, this should
be cleaned up at some point in the future (new bug??). Unfortunately the
way numberOperate works this is ugly, maybe we just call a new function to
print the assignment after any numberOperate?

Old: set num ?32?shf on ...
New: set num 32'h0 on ...

Another thing for that new bug might be to pull user2/3, m_const*, and the
related value accessors into a separate support class e.g. SimulateValues
that the visitor references. Right now the visitor functions poke at too
many internals related to value resolution, and pulling this out would make
the class easier to understand/maintain.

All this work will likely collide with any edits towards #�.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 9, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-09T22:06:11Z


Re: t_lint_width_genfor, I'm thinking the WIDTH warning in the procedural block "ri = i;" may not really be new. I went back to the revision mentioned and the genvar warnings went away, but the procedural warning is still there. I'm going back and have so far found it back as far as v4.002:

%Warning-WIDTH: t/t_lint_width_genfor.v:29: Operator ASSIGN expects 4 bits on the Assign RHS, but Assign RHS's VARREF 'i' generates 32 bits.
%Warning-WIDTH: Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message

Also, the procedural assignment would seem to be a different class of problem. I can definitely see how V3Simulate could tell you if your truncating while verilating. Proving that a behavioral assignment cannot truncate seems like a whole different ballgame.

Unless there's an objection, I'm going to assume that the procedural WIDTH warning is legitimate.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-10T05:07:57Z


Ok, the new test was written without looking at the old warning rules. My thinking was any const assignment to a unranged thing (int etc) would not warn.

But the old behavior seemed to work well, so I'd suggest we can just lint off out that (new) broken violation in the test so it'll pass.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 12, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-12T13:14:26Z


Here is my proposed fix:
https://github.com/toddstrader/verilator-dev/tree/genvar-width

It appears the specific issue in the work towards #� was that it was discarding widthSized. As expected, some of the error message formatting has been changed around constant printing (actually, it's just been changed back to what it was before #�).

I had to remove an assertion I put in allocConst() which compared dtypes of the recycled node and the new const. This assertion doesn't work anymore because it needs the value to be set for widthMin to agree. But that doesn't happen until outside of allocConst(). The code could be refactored to make this happen, but now that I've dug deeper into this logic, I don't really see the value in the assertion anymore.

This doesn't appear to have disturbed #� as t_enum_size is still passing.

Also, I checked and this resolved the SweRV test failure in verilator_ext_tests.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 12, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-12T19:26:53Z


Great, tighter patch than I was expecting. Please squash and push.
& if have chance (if didn't already), check extended tests.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 12, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-12T20:17:03Z


Squashed, pushed and CI is green.

Verilator_ext_tests is working too. That CI environment is still a WIP, but is basically working in my branch:
https://travis-ci.com/toddstrader/verilator_ext_tests/builds/122567524

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 29, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-29T23:15:13Z


In 4.018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.