OptimizeInstructions: Do not remove an inner cast when there are descriptor effects in the way - #9067
Conversation
| ShallowEffectAnalyzer movingEffects(options, *getModule(), curr->ref); | ||
| if (movingEffects.orderedBefore(descEffects)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I don't understand why this is correct. We're replacing the current ref argument (which is evaluated before the desc) with the child's ref argument (which is also evaluated before the desc). So what's being reordered?
There was a problem hiding this comment.
The trap due to the inner cast. See the test, which is this:
(outer.cast
(inner.cast (inner.ref))
(descriptor with effects)
)
=>
(outer.cast
(inner.ref)
(descriptor with effects)
)
Now inner.cast is gone. Before, if it trapped, it trapped before the descriptor's effects. Now, the outer cast still traps - we didn't lose the trap - but it is after the descriptor.
There was a problem hiding this comment.
I see, it's not the expressions that are being reordered; it's the trap effect. And trapping is the only effect that matters here because we already know that curr->ref is another cast. It would be helpful to call that out slightly more explicitly in the comment.
Also, we should combine this new logic into the calculation of safe above. This is more than a nice refactoring because returning early here prevents later patterns from being evaluated.
There was a problem hiding this comment.
Thanks, comment expanded and logic reordered.
2 participants
No description provided.