Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1487

New WIDTH warnings on genvars

Added by Wilson Snyder 2 months ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Lint
% Done:

0%


Description

The following in git (unreleased) creates a warning where previously it did not, due to fixing bug1442. 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

History

#1 Updated by Wilson Snyder 2 months ago

  • Description updated (diff)

#2 Updated by Wilson Snyder 2 months ago

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 bug1442 relating to widthing parameters, but was the V3Number constant changes towards bug1315 in commit b045111a67c39c3b41abfec74ca9779f7df0b201. The previous commit 654571bd1cc02c1d4c1f4ed3ac23c21deee50e10 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 bug1442 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 bug1442 might break again with this stuff fixed, IMO this bug is more important than bug1442, so ok to fix this and reopen bug1442. For bug1442 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 bug1315.

#3 Updated by Todd Strader 2 months ago

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.

#4 Updated by Wilson Snyder 2 months ago

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.

#5 Updated by Todd Strader 2 months ago

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

It appears the specific issue in the work towards bug1315 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 bug1315).

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 bug1442 as t_enum_size is still passing.

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

#6 Updated by Wilson Snyder 2 months ago

  • Category set to Lint
  • Assignee changed from Wilson Snyder to Todd Strader

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

#7 Updated by Todd Strader 2 months ago

  • Status changed from Assigned to Resolved

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

#8 Updated by Wilson Snyder about 2 months ago

  • Status changed from Resolved to Closed

In 4.018.

Also available in: Atom