[Custom Descriptors] Heap2Local: Handle definitely-failing descriptor casts #7919
Conversation
kripken
changed the title
|
Actually, after this, no tests remain that check the nullable case. That |
|
(making the altered tests use a nullable cast does not reach that code path, for some reason) |
test/lit/passes/heap2local-desc.wast
Outdated
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (local.get $desc) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: (unreachable) |
There was a problem hiding this comment.
It seems bad that this successful cast is being optimized to unreachable!
src/passes/Heap2Local.cpp
Outdated
| if (curr->type.isNonNullable()) { | ||
| // No null is possible; trap. | ||
| replaceCurrent(builder.blockify(builder.makeDrop(curr->ref), | ||
| builder.makeDrop(curr->desc), | ||
| builder.makeUnreachable())); |
There was a problem hiding this comment.
Ideally we should be able to optimize out an allocation even if it flows into a successful non-null descriptor cast. This code is not correct because it replaces such a successful cast with an unreachable, though. If we did the correct optimization, the cast (which returns a non-null reference) would be replaced with a propagation of the null value. Since this is not a simple type refinement, we would have to guarantee that whatever location receives the null value will also be updated to accept it.
|
Ok, yes, I was totally misunderstanding this. The null here wasn't the result of a successful cast. It is the "empty" value we carry around when we know an allocation is not actually needed (but we need some value). The real problem here is that the |
kripken
changed the title
src/passes/Heap2Local.cpp
Outdated
| builder.makeRefNull(allocation->type.getHeapType()), | ||
| builder.makeUnreachable()))); | ||
| if (!Type::isSubType(allocation->type, curr->type)) { | ||
| // The cast fails, so it must trap. |
There was a problem hiding this comment.
Maybe extend the comment with something like this:
"We mark such failing casts as fully consuming their inputs, so we cannot just emit the explicit descriptor equality check below because it would appear to be able to propagate the optimized allocation on to the parent."
When moving a
ref.cast_desc's reference into locals, we didn't handle thenon-nullable case. We must trap there and not return nullref.
(The pass handled the descriptor in this way, but not the reference.)
Diff is slightly smaller without whitespace.
edit: see real reason below