Handle exactness in MinimizeRecGroups #7555
Conversation
Use the standard utility rather than reimplementing type intersection. The new code is simpler, shorter, and properly supports exactness, avoiding an assertion failure in the added test case. The other functional change is that when one of the intersected heap types is bottom and the type GLB is a non-nullable reference to bottom, the result of the intersection is `None` where it was previously a `Cone`.
Previously doing e.g. `type.with(HeapType::none)` would cause an assertion failure if `type` was exact because `.with()` would only replace the heap type and exact references to basic heap types are disallowed. Rather than checking for and avoiding this error in all the callers, simply drop exactness when `.with()` is called with a basic heap type. This is reasonable behavior because the only alternative is never correct. Add a test that hits an assertion failure without this fix. AbstractTypeRefining replaces a defined type with `none` and the type updating utility does not check whether the new heap type is basic before doing the replacement.
When custom descriptors are disabled, validate that public types do not contain exact references. If they did, we would drop the exactness and change the identity of the public type during binary writing, which would be incorrect. This still allows internal usage of exact types without custom descriptors enabled, and it is up to the individual passes to ensure that the eventual erasing of exactness does not cause any problems.
Treat rec groups differing only in the exactness of a reference as different, but only when custom descriptors is enabled. When custom descriptors is not enabled, exactness will be erased before the binary is written, so if two minimized rec groups differed only in exactness, they would in fact be written as the same rec group. This would change the behavior of casts meant to differentiate between types in that rec group, so it would be incorrect.
|
Were we not thinking that we could first measure the benefit of lifting/optimizing/lowering exact types when exact types are not enabled, and only after that, consider the work to make all the transforms etc. be modulo exact types? I recall that was the direction we discussed but maybe we didn't fully agree? Or is this necessary for the measurement somehow? (but it seems like we could measure first, ignoring possible bugs like this, as an upper bound?) |
|
Yes, I think that investigation makes sense, but this pass is special because its optimization could easily be partially or fully undone by any following type rewriting. Any type rewriting pass that tries to avoid undoing rec group minimization will have to share quite a bit of the conflict resolution logic with rec group minimization anyway, so there will be no way around updating the wasm-type-shape utilities to take features into account either way. |
|
I'm generally in favor of ignoring bugs like this for now (and in particular I'm in favor of ignoring the class of bugs where a sequence of type optimizations followed by lowering causes conflicts), but the fuzzer found this bug fairly easily, so I think it's worth fixing. |
Wait, do we not expect rec group minimization to happen at the very end anyhow? (in which case no type rewriting happens later) What would be the benefit of minimizing rec groups in the middle? |
|
Oh, you're thinking that first we would run the lowering pass to remove exactness and then afterward we would minimize rec groups? That means that the separate lowering pass would have to be run explicitly, which would require users to know to run it. It would also complicate validation, since whether allocations are expected to be exact or not would depend on whether lowering has happened or not. |
|
It does seem simpler to lower first and then minify. One option would be to require the lowering to be done explicitly, but if we minify in Alternatively, if users are expected to explicitly minify, then having them explicitly lower seems reasonable as well. (We could guide them to it by the minify pass erroring clearly.) If we aren't certain how this will end up (and we do still need to measure the benefit of non-CD exactness), then doing it explicitly for now might make sense? |
|
@kripken, is this ready for a second look, given our offline discussion? |
There was a problem hiding this comment.
For the record here, offline @tlively convinced me that it makes more sense to optimize modulo exactness (ignore exactness when the feature is not enabled), then strip away exactness in binary writing, than the option of a proper lowering pass. While we do need to consider exactness in the optimizer, otoh this approach lets the IR look the same whether or not the feature for exactness is present (avoiding a situation where say struct.new would be typed differently in the IR, based on features, which could be noticed in many places).
So this approach keeps the IR simpler even if it does add another invariant to keep in mind. Overall it is simpler I think.
@tlively please document this new invariant on the IR in the readme section on the IR, if we haven't already.
Treat rec groups differing only in the exactness of a reference as
different, but only when custom descriptors is enabled. When custom
descriptors is not enabled, exactness will be erased before the binary
is written, so if two minimized rec groups differed only in exactness,
they would in fact be written as the same rec group. This would change
the behavior of casts meant to differentiate between types in that rec
group, so it would be incorrect.