Before this commit, the DLAStep DeduplicateFields was using
NotMergedNode (a reference) in an assertion, in a place where it was
already invalidated by a preceding call to
CollapseSingleChild::collapseSingle that can mutate the graph.
This caused the assertion to trigger erroneously in some situations.
This commit moves the assertion earlier, in a place where the reference
is guaranteed to always be valid.
Before this commit the breadth-first traversal was more convoluted and
it used a broken criterion for bailing out, resulting sometimes in
considering a pointer edge as if it was an instance edge.
This in turn caused bugs to the point of the DLA Graph failing
verification.
This commit rewrites and simplifies the main traversal and comparisons
in DeduplicateField to make them robust against these bugs.
Before this commit postProcessMerge was making some strong assumptions
about dla::CollapseSingleChild::collapseSingle that no longer hold.
Namely the (now) wrong assumption is that collapseSingle would not break
post_order iteration, which is now false because collapseSingle can
change the incoming edges of the node it's called on, and those incoming
edges are on the visit stack of the post_order iteration itself.
This required making a copy of the post order, and resulted in dropping
the postProcessMerge function alltogether.
A logic bug caused the wrong detection of nodes with many fields.
As a result, the field deduplication was only executed on a subset of
the real candidates.
This caused the following two problems.
- Degradation of the quality of the results, i.e. unions that could be
deduplicated were not deduplicated.
- Failing assertions in rare cases.
The algorithm assumes that if it's looking at the children
(C1, ..., Cn) of node A to be deduplicated, then all (C1, ..., Cn)
have beed already visited, hence they don't have children that should
be duplicated because they would have been deduplicated already, when
looking at (C1, ..., Cn). The bug possibly caused to miss the
deduplication of children of Ci (for some i) causing assertions to
fail when looking at A to deduplicate (C1, ..., Cn).
Before this commit, the dla::Step depended on InterferingInfo, but this
was superfluous.
This commit drops the dependency and enables the Step to run before
ComputeNonInterferingComponents.
This also avoids the need to run ComputeNonInterferingComponents twice
(one before and one after DeduplicateUnionFields), so it can only run
once.
Alessandro Di Federico