[Custom Descriptors] Fix exact types in RemoveUnusedBrs handling of SuccessOnlyIfNonNull #7615
Conversation
src/passes/RemoveUnusedBrs.cpp
Outdated
| // Given the RefCast we are forced to use, we are replacing a | ||
| // BrOn* with a BrOnNonNull + a RefCast, which is never better | ||
| // than any BrOn* by itself (even BrOnCast). | ||
| return; |
There was a problem hiding this comment.
br_on_non_null has a couple benefits over br_on_cast: first, it takes at least three bytes less to encode (but usually more like 5 bytes less to encode) because its opcode does not have a prefix and it does not need the two heap type immediates that br_on_cast has. Even accounting for the extra two bytes and one heap type immediate required to encode the ref.cast, this is a code size savings in the usual case and a wash in the worst case. Second, splitting the cast out of the branch lets OptimizeInstructions and other passes get to it, which in principle can lead to better follow-on optimizations (although I don't know for sure that it does so).
There was a problem hiding this comment.
Hmm, but you need to add the ref.null (line 1025), adding 2 bytes. Though, maybe that can lead to optimizations..?
There was a problem hiding this comment.
Good point, although, yes, I hope that would go away after follow-on optimizations. Anyway I don't feel too strongly about this (nor do I have data to support a decision). LGTM if you suspect this change will be better overall.
There was a problem hiding this comment.
Hmm, I don't feel strongly either. I flipped it to optimize, as your intuition makes sense to me.
In that case we dropped exactness, leading to an error on the new testcase
after optimization. Fix is to use
.with(Nullable).However, it is better to skip optimization here: replacing a BrOn with another
BrOn + a RefCast is not any better. This also alters an existing test.