Fix isorecursive canonicalization #5269
Conversation
Fixes a longstanding problem with isorecursive canonicalization that only showed up in MacOS and occasionally Windows builds. The problem was that `RecGroupEquator` was not quite correct in the presence of self-references in rec groups. Specifically, `RecGroupEquator` did not differentiate between instances of the same type appearing across two rec groups where the type was a self-reference in one group but not in the other. The reason this only showed up occasionally on some platforms was that this bug could only cause incorrect behavior if two groups that would incorrectly be compared as equal were hashed into the same bucket of a hash map. Apparently the hash map used on Linux never hashes the two problematic groups into the same bucket.
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| return groupA == groupB || (groupA == newGroup && groupB == otherGroup); | ||
| bool selfRefA = groupA == newGroup; | ||
| bool selfRefB = groupB == otherGroup; | ||
| return (selfRefA && selfRefB) || (!selfRefA && !selfRefB && groupA == groupB); |
There was a problem hiding this comment.
I've forgotten how this code works, sorry. Is newGroup a new group being built that is being compared to an existing group otherGroup? How can groupA/B not match those two, why would we be comparing types not from the groups at all, when comparing newGroup/otherGroup?
I couldn't find code comments explaining how the class RecGroupEquator is used - are they somewhere else than on the class itself?
There was a problem hiding this comment.
I've forgotten how this code works, sorry. Is newGroup a new group being built that is being compared to an existing group otherGroup?
Yes, exactly.
How can
groupA/Bnot match those two, why would we be comparing types not from the groups at all, when comparingnewGroup/otherGroup?
Because we might be comparing heap types that are used in newGroup and otherGroup (e.g. because structs defined in those groups contain reference to the heap types), but are defined in some arbitrary different previous groups. We're trying to compare the "shapes" of newGroup and otherGroup, and how they reference other groups is part of that "shape."
I couldn't find code comments explaining how the class RecGroupEquator is used - are they somewhere else than on the class itself?
There are some additional comments on RecGroupStructure and RecGroupStore.
This would be difficult since |
|
sgtm, a re-enabled flaky test that is now stable seems enough. |
Fixes a longstanding problem with isorecursive canonicalization that only showed
up in MacOS and occasionally Windows builds. The problem was that
RecGroupEquatorwas not quite correct in the presence of self-references inrec groups. Specifically,
RecGroupEquatordid not differentiate betweeninstances of the same type appearing across two rec groups where the type was a
self-reference in one group but not in the other.
The reason this only showed up occasionally on some platforms was that this bug
could only cause incorrect behavior if two groups that would incorrectly be
compared as equal were hashed into the same bucket of a hash map. Apparently the
hash map used on Linux never hashes the two problematic groups into the same
bucket.