[Custom Descriptors] Update Heap2Local #7667
Conversation
When optimizing allocations of structs with descriptors, store the descriptor in a local the same way we store normal fields. Update ref.get_desc on optimized allocations to simply get the local holding the descriptor. Lower ref.cast_desc to unreachable when the optimized allocation flows in as the descriptor because it cannot have also been the descriptor on the cast value without being considered to have escaped. When the optimized allocation is itself the cast value, compare the local containing its descriptor to the target descriptor to determine whether the cast would have succeeded.
src/passes/Heap2Local.cpp
Outdated
| // remove the allocation it will not even be needed for validation. | ||
| replaceCurrent(curr->ref); | ||
| if (curr->desc) { | ||
| if (analyzer.getInteraction(curr->ref) == ParentChildInteraction::Flows) { |
There was a problem hiding this comment.
Why is this checking the interaction? I can't figure that out. What other interaction is being ruled out?
There was a problem hiding this comment.
We're optimizing an allocation that flows into the cast either as the ref or as the desc. This intends to differentiate between those two cases.
(I guess maybe I also have to check the case where the same allocation flows into both locations...)
There was a problem hiding this comment.
Ah, right, thanks. Makes sense.
(I think the case of flowing into both is already handled, by the first case?)
There was a problem hiding this comment.
It was, but not optimally, and adding a test revealed a segfault when the allocation target does not have a descriptor. PTAL at the fixes.
| (ref.cast_desc (ref (exact $super)) | ||
| (local.get $ref) | ||
| (struct.new $super.desc) | ||
| ) |
There was a problem hiding this comment.
Side question about the spec, why is this instruction called ref.cast_desc and given a type to cast to, if what it actually does is compare the desc for equality? Seems like ref.eq_desc..?
There was a problem hiding this comment.
If all you want to do is check equality of descriptors, you can already do that with ref.get_desc and ref.eq. If that check succeeds, you know what type the described value must have, but the type system does not have that information. So the main point of this instruction is to be a cast, i.e. to check that a value has a particular type and use that type in further validation. It just happens to implement that cast by checking equality of descriptors.
There was a problem hiding this comment.
I see, thanks. The name seems slightly odd but I get the use case.
src/passes/Heap2Local.cpp
Outdated
| // If we are doing a ref.cast_desc of the optimized allocation, but we | ||
| // know it does not have a descriptor, then we know the cast must fail. We | ||
| // also know the cast must fail if the optimized allocation flows in as | ||
| // the descriptor, since it cannot possible have been used in the |
There was a problem hiding this comment.
| // the descriptor, since it cannot possible have been used in the | |
| // the descriptor, since it cannot possibly have been used in the |
There was a problem hiding this comment.
Wow my typing correctness has really gone downhill recently 😓
|
Looks like CI found some unrelated UB when running the clusterfuzz tests: |
When optimizing allocations of structs with descriptors, store the
descriptor in a local the same way we store normal fields. Update
ref.get_desc on optimized allocations to simply get the local holding
the descriptor. Lower ref.cast_desc to unreachable when the optimized
allocation flows in as the descriptor because it cannot have also been
the descriptor on the cast value without being considered to have
escaped. When the optimized allocation is itself the cast value, compare
the local containing its descriptor to the target descriptor to
determine whether the cast would have succeeded.