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

Fix assertion failure in bitOpTree opt #2899

Merged
merged 3 commits into from
Apr 25, 2021

Conversation

yTakatsukasa
Copy link
Member

Second try to repair #2891

Ast such as below caused the issue:

     XOR 0x55aed9e5b430 <e104786#> {d70bs} @dt=0x55aed9c4e050@(G/wu32/1)
    1: CCAST 0x55aeda436a10 <e147374#> {d70bs} @dt=0x55aed9c4e050@(G/wu32/1) sz32
    1:1: SHIFTR 0x55aed9e5aec0 <e147371#> {d70bs} @dt=0x55aed9d80f80@(G/wu64/33)
    1:1:1: CCAST 0x55aed9e5af80 <e147361#> {d30cl} @dt=0x55aed9d80f80@(G/wu64/33) sz64
    1:1:1:1: WORDSEL 0x55aeda07ea30 <e336940#> {d30cn} @dt=0x55aed9c4d170@(G/w32)
    1:1:1:1:1: VARREF 0x55aeda435af0 <e147260#> {d30cn} @dt=0x55aed9c3ff20@(w128)p[3:0]  t__DOT__data_i [RV] <- VAR 0x55aed9e48c70 <e2223#> {d23aw} u4=0x1 @dt=0x55aed9c3ff20@(w128)p[3:0]  t__DOT__data_i [VSTATIC]  VAR
    1:1:1:1:2: CONST 0x55aeda07e0a0 <e147261#> {d30ct} @dt=0x55aed9c4d170@(G/w32)  32'h0
    1:1:2: CONST 0x55aed9e59d60 <e147362#> {d70bs} @dt=0x55aed9c4d170@(G/w32)  32'h20
    2: CCAST 0x55aeda436ad0 <e147394#> {d70bs} @dt=0x55aed9c4e050@(G/wu32/1) sz32
    2:1: SHIFTR 0x55aed9e5adf0 <e147391#> {d70bs} @dt=0x55aed9d80f80@(G/wu64/33)
    2:1:1: VARREF 0x55aed9e5a110 <e147381#> {d70bs} @dt=0x55aed9d80f80@(G/wu64/33)  t__DOT__genblk1__BRA__0__KET____DOT__u_bank_data_ecc_check__DOT____Vtogcov__data_i [LV] => VAR 0x55aed9f7dec0 <e27062#> {d70bs} @dt=0x55aed9c3d320@(G/w33)  t__DOT__genblk1__BRA__0__KET____DOT__u_bank_data_ecc_check__DOT____Vtogcov__data_i MODULETEMP

The bitwidth of a variable considering casts in tree requires traversing backp().
Instead of doing so, the width of m_bitPolarity is dynamically expanded if necessary.

I ran ext_tests with --trace and --coverage enabled.

src/V3Const.cpp Outdated
if (bit >= m_bitPolarity.width()) {
const V3Number oldPol = std::move(m_bitPolarity);
// oldPol.width() is 8, 16, or 32 because this visitor is called after V3Expand
const int newWidth = oldPol.width() * 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the * 2 for? Below you are only initing up to width, not width*2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is expanding m_bitPolarity.
So newWidth must be greater than bit + 1.
Expanding to bit + 1 is maybe ok, but expanding by 2x is better because

  • CCast will cast to such bitwidth anyway (33 bit width has to be 64 in C++)
  • can avoid frequent expansion

The expanded m_bitPolarity is filled by 'X', then copy from oldPol .

verilator/src/V3Const.cpp

Lines 120 to 126 in 6af31c3

m_bitPolarity.setAllBitsX();
for (int i = 0; i < oldPol.width(); ++i) {
if (oldPol.bitIs0(i))
m_bitPolarity.setBit(i, '0');
else if (oldPol.bitIs1(i))
m_bitPolarity.setBit(i, '1');
}

This is matches the initialization inf ctor.

verilator/src/V3Const.cpp

Lines 179 to 183 in 6af31c3

VarInfo(ConstBitOpTreeVisitor* parent, AstVarRef* refp)
: m_parentp(parent)
, m_refp(refp)
, m_bitPolarity(refp, refp->isWide() ? VL_EDATASIZE : refp->width()) {
m_bitPolarity.setAllBitsX();

Thanks for highlighting this code. I noticed that m_bitPolarity has to be expanded more than 2x in some cases. (e.g. 8 to 32)
So I pushed 6af31c3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wsnyder Does the comment above answer your question ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks

@RaynardQiao
Copy link
Contributor

With the latest master , All testcase passed. #2891
thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants