Fix fallthrough for desc operands - #9089
Conversation
`getImmediateFallthroughPtr` needs to make sure that the fallthrough value is not modified or otherwise rendered unusable by the side effects of expressions that are evaluated after the fallthrough expression but before the top-level expression. Previously the only such expression was the br_if condition, and `getImmediateFallthroughPtr` only looked through the br_if if the condition did not have side effects that would interfere with the rest of the fallthrough expression. Make this effect analysis slightly more precise by using `orderedBefore` instead of `canReorder`. Also add similar effects analysis for the `desc` operands of `RefCast` and `BrOn` expressions, if it exists. These operands are also evaluated after the fallthrough expression. Finally, update `areConsecutiveAndEqual` in `OptimizeInstructions` to look through `RefCast` and `BrOn` expressions despite any effects that may occur in their `desc` operands; it will do its own more precise effects analysis afterward.
| ) | ||
| ) | ||
| ) | ||
| (i32.const -1) |
There was a problem hiding this comment.
Does this function need a return value?
There was a problem hiding this comment.
This goes along with the return of the ref.eq. The test would work without the return, but the benefit of having the return value is that it will make bugs much more obvious when this function is used as a fuzzing seed. With the return values present, --fuzz-exec is able to tell that these functions are misoptimized without the fixes in this PR.
I can remove it if you prefer, though, since it's not immediately useful in the test itself.
There was a problem hiding this comment.
Oh, I see. Fair enough, sgtm to keep it.
| ;; NTRAP-NEXT: (i32.const -1) | ||
| ;; NTRAP-NEXT: ) | ||
| (func $br_on_cast_desc_eq_fail-side-effects-in-desc (param $x (ref null $struct)) (param $d (ref null $desc)) (result i32) | ||
| (drop |
There was a problem hiding this comment.
Please add a comment explaining what is tested here.
There was a problem hiding this comment.
Done. Copied the comments from the above test variants. The only thing different here is that these three tests use the *_fail variant of the instruction.
2 participants
getImmediateFallthroughPtrneeds to make sure that the fallthrough value is not modified or otherwise rendered unusable by the side effects of expressions that are evaluated after the fallthrough expression but before the top-level expression. Previously the only such expression was the br_if condition, andgetImmediateFallthroughPtronly looked through the br_if if the condition did not have side effects that would interfere with the rest of the fallthrough expression.Make this effect analysis slightly more precise by using
orderedBeforeinstead ofcanReorder. Also add similar effects analysis for thedescoperands ofRefCastandBrOnexpressions, if it exists. These operands are also evaluated after the fallthrough expression.Finally, update
areConsecutiveAndEqualinOptimizeInstructionsto look throughRefCastandBrOnexpressions despite any effects that may occur in theirdescoperands; it will do its own more precise effects analysis afterward.