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 #�.

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.

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.

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.

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.

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

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

In 4.018.