Wasmtime GitHub notifications bot (Apr 16 2022 at 03:52): Wasmtime GitHub notifications bot (Apr 16 2022 at 03:52):cfallin opened PR #4044 from fix-empty-brtable-fuzzbug to main:
regalloc2 is a bit pickier about critical edges than regalloc.rs was,
because of how it inserts moves. In particular, if a branch has any
arguments (e.g., a conditional branch or br_table), its successors must
all have only one predecessor, so we can do edge moves at the top of
successor blocks rather than at the end of this block. Otherwise, moves
that semantically must come after the block's last uses (the branch's
args) would be placed before it.This is almost always the case, because crit-edge splitting ensures that
if we have more than one succ, all our succs will have only one pred.
This is because branch kinds that take arguments (fixed args, not the
blockparam args) tend to have more than one successor: conditionals and
br_tables.However, a fuzzbug recently illuminated one corner case I had missed: a
br_table can have one successor only, if it has a default target and
an empty table. In this case, crit-edge splitting will happily skip a
split and assume that we can insert edge moves at the end of the block
with the br_table. But this will fail.regalloc2 explicitly checks this and bails with a panic, rather than
continue, so no miscompilation is possible; but without this fix, we
will get these panics on br_tables with empty tables.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Apr 16 2022 at 03:52):cfallin requested alexcrichton for a review on PR #4044.
Wasmtime GitHub notifications bot (Apr 16 2022 at 03:52):cfallin requested fitzgen for a review on PR #4044.
Wasmtime GitHub notifications bot (Apr 18 2022 at 16:29):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 18 2022 at 16:29):fitzgen created PR review comment:
I'm not seeing how a
br_tablewith no table and just a default label has an outgoing edge count of two.The explanation you wrote in the PR description makes sense to me, but I'm having trouble matching that with what is written/happening here. I felt like I understood the problem when reading the PR description, but looking at the fix I'm now lost again.
Is it that there is some conservative over-estimating happening here? If so, then I think this comment needs to better describe what is going on. If not, then I think this comment also needs to better describe what is going on :-p
Wasmtime GitHub notifications bot (Apr 18 2022 at 16:29):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:04):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:04):cfallin created PR review comment:
Err, this was a way to force the edge-splitting not to assume it can add moves at the end of this block (there aren't actually two successors, but if we say there are, then the logic moves onto next options); but I agree the comment could be clearer! I'll update.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:17):cfallin updated PR #4044 from fix-empty-brtable-fuzzbug to main.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:18):cfallin updated PR #4044 from fix-empty-brtable-fuzzbug to main.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:27):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 18 2022 at 17:59):cfallin merged PR #4044.
Last updated: Nov 22 2024 at 16:03 UTC