Make node iteration stricter #4617
Conversation
|
Note: I will review code involving addHereThisAsNext if you hare happy with it otherwise |
| std::function<bool(AstNode*)> isRecursive = [&](AstNode* np) -> bool { | ||
| return np && np->exists([&](AstNodeVarRef* refp) { | ||
| return refp->varp() == nodep || isRecursive(refp->varp()->valuep()); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
I preferred the old form as if we ever have another way to get to the var twice it will detect, and also IMO was easier to understand. Was this needed to prevent a new assertion?
There was a problem hiding this comment.
This is needed because of the assertions: you can't call iterateAndNext on the same node twice (recursively). Doing so was never really safe to do due to the later call stomping m_iterpp hence edits in the higher call would have been borked.
| // Simplify the value | ||
| iterate(itemp->valuep()); |
There was a problem hiding this comment.
This may change itemp, so you shouldn't record itemp in a variable. (Generally true throughout V3Width - avoid remembering node pointers across iterate.)
There was a problem hiding this comment.
This is V3Const, and we are iterating the the child node valuep(), not itemp? In this case, we know itemp is not changed as it is an AstEnumItem, which should not ever be changed, only it's childern (the enum vlaue)?
|
I have thought of a way to make it all work with the old behaviour of At the same time, if the old node did not happen to be iterated, then the new one would not be either (if inserted earlier in the tree), so whether the inserted node is iterated is not obvious. I think it would more likely catch bugs where a recursive iteration is missing if the iterator was not fixed up (like with this patch), so you are more likely to realize using simpler tests that a recursive iteration is needed. For example, a recursive iteration was missing in V3Premit when the temporary is introduced in a while loop condition, resulting in a ShiftR not being converted to a ShiftROvr. With this PR applied, the replacement is performed:
With that in mind I'm leaning towards not fixing up the iteration on |
This is a prep patch for an experiment on removing m_iterpp (which I have in a hacked up form, and does not actually help much, 1.2% memory saving, yay...). Even if we don't land that, I think we want these changes that makes iteration a little bit stricter. Notably, there are assertions (only under VL_DEBUG, due to being in hot code), that ensure that we don't recursively iterate the same node. We used to do this in a few places (which I fixed), but this is generally not safe due to the recursive call to iterateAndNext overwriting m_iterpp, and hence any subsequent edits would be unsafe. Apart from the direct recursion, we also need to prevent addHereThisAsNext from updating the iteration pointer. I think this is a good thing. Previously it used to force iteration to restart on the nodes inserted *before* the currently iterated node, which would also mean that the currently iterated node would then be visited again, as it became a successor of the inserted node. With this patch, iteration continues with the edited node only if it's replacing the current node, or if the new node is a successor (nextp) of the current node, but not if the edit is adding the node before the current node. I fixed the few places (V3Premit, V3Task) where we used to rely on the old behaviour. The fix is simply to explicitly iterate the new nodes you insert before the current node. Patch is performance neutral.
gezalore
force-pushed
the
stricter-iter
branch
from
56b0afd
to
3a25e09
Compare
|
I'd lean towards reiterating on addHereAsNext, because it seems more similar to the "replace" and is less risky. E.g. the insert might have a subtree that was not pre-visited and still needs to be. The downside is mainly performance, which is reasonable sacrifice. The general throwing of an assert on double-visit scares me a bit as I suspect there will be fallout, but with all assertions it does seem better to get these cleaned up. |
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from verilator#4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from verilator#4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from verilator#4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from verilator#4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from verilator#4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from #4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
- Enable creating constant pool entries for RHS of simple var = const assignments - Never extract ArraySel (it's just pointer arithmetic) - Remove unnecessary AstTraceInc precond child tree - Always fully recurse expressions (fix transplanted from #4617) - General cleanup Overall the patch is performance neutral to slightly positive, but saves ~10% peak Verialtor memory usage due to not creating temporaries (which are later expanded) for any ArraySels.
This is a prep patch for an experiment on removing m_iterpp (which I have in a hacked up form, and does not actually help much, 1.2% memory saving, yay...). Even if we don't land that, I think we want these changes that makes iteration a little bit stricter. Notably, there are assertions (only under VL_DEBUG, due to being in hot code), that ensure that we don't recursively iterate the same node. We used to do this in a few places (which I fixed), but this is generally not safe due to the recursive call to iterateAndNext overwriting m_iterpp, and hence any subsequent edits would be unsafe.
Apart from the direct recursion, we also need to prevent addHereThisAsNext from updating the iteration pointer. I think this is a good thing. Previously it used to force iteration to restart on the nodes inserted before the currently iterated node, which would also mean that the currently iterated node would then be visited again, as it became a successor of the inserted node. With this patch, iteration continues with the edited node only if it's replacing the current node, or if the new node is a successor (nextp) of the current node, but not if the edit is adding the node before the current node. I fixed the few places (V3Premit, V3Task) where we used to rely on the old behaviour. The fix is simply to explicitly iterate the new nodes you insert before the current node.
Patch is performance neutral.