[RFC] Fix pure-ness check

[yTakatsukasa]

While I am writing a small RTL to improve code coverage (#2824), I found that any node is considered "pure" because m_doShort is always true.

I guess the original intention is like 31aa715.

Unfortunately I found some tests fail after the change.

• statistic mismatch (less optimized)
• result mismatch, (latched? )
• Internal error
  The former one is predicted, but the other two are not acceptable.

Should I add recursive pure-ness check ? ( Not so difficult, but not sure for its cost).

Adding rough check (11c4361) fixes tests.
The visitor is not perfetct.

• no caching
• treat any task/function call is non-pure (too pessimistic)
• any uniop. biop, triop, quadop are considered pure (can be too optimistic, but not sure)

[wsnyder]

I think the immediate patch would be just to remove m_doShort.

There's a great deal of work to do to support true short-circuiting, so don't think this smaller patch just looking at improving isTPure by itself is worth doing without hitting the larger issues. If you'd like to take on short circuiting generally that would be awesome, we should discuss in #487.

[yTakatsukasa]

Unfortunately just ignoring m_doShort is not sufficient.
31aa715 does so. (m_doShort is still there thought).

There are several test failures other than statistic mismatch, such as:
https://github.com/verilator/verilator/runs/2211756144#step:6:427 (Verilator crashes by an internal error)

It seems there are some optimizations that the latter pass requires. I will look for minimal change.

[yTakatsukasa]

My experiment shows the following check is minimal to pass the existing tests. Note that I am not saying that I want to merge the check to master.

verilator/src/V3Const.cpp

Lines 916 to 929 in b891dab

	if (!m_doShort) return false;
	if (VN_IS(nodep, VarRef) || VN_IS(nodep, Const)) return true;
	if (AstNot* notp = VN_CAST(nodep, Not)) return isTPure(notp->lhsp());
	if (AstWordSel* selp = VN_CAST(nodep, WordSel))
	return isTPure(selp->fromp()) && isTPure(selp->bitp());
	if (AstNodeBiop* biopp = VN_CAST(nodep, And))
	return isTPure(biopp->lhsp()) && isTPure(biopp->rhsp());
	if (AstNodeBiop* biopp = VN_CAST(nodep, Or))
	return isTPure(biopp->lhsp()) && isTPure(biopp->rhsp());
	if (AstNodeBiop* biopp = VN_CAST(nodep, Eq))
	return isTPure(biopp->lhsp()) && isTPure(biopp->rhsp());
	if (AstNodeTriop* triopp = VN_CAST(nodep, Sel))
	return isTPure(triopp->lhsp()) && isTPure(triopp->rhsp()) && isTPure(triopp->thsp());
	return false;

These changes are necessary to get correct simulation result. (ShiftL with excessive shift amount has to be removed because it is undefined behavior in C++). Checks in b891dab do not guarantee to show correct result for any design. Current istPure() that always returns true looks to give correct simulation result in general.

I am feeling to just add test (629e064) marked fail for now.

[wsnyder]

Hmm. I think the newer code is closer to the right path, but agree marking the test fail for now is probably the safest path until we have time to think this out completely.

[yTakatsukasa]

I'd like to close this PR for now. Hopefully I will tackle again.