bytecodealliance / regalloc2

A new register allocator
Apache License 2.0
218 stars 38 forks source link

Fuzzer Not Detecting Incorrect Allocation #191

Open d-sonuga opened 2 months ago

d-sonuga commented 2 months ago

It's possible for vregs to be used or defined in branch instructions, but the fuzzer doesn't seem to check for these operands.

The following doesn't pass the fuzzer:

block0:
 0. branch(1). operands: [def v0 (fixed: p0)] // Allocation: [v0: p0]

block1:
 1. operands: [use v0 (fixed: p0)] // Allocation: [v0: p0]

And this does:

block0:
 0. operand: [def v0 (fixed: p0)] // Allocation: [v0: p0]
 1. branch(1). operands: [use v0 (fixed: p1)] // Allocation: [v0: p9]

block1:
 2. operands: [use v0 (fixed: p0)] // Allocation: [v0: p0]

Which is incorrect.

To reproduce this, run the tests in src/fastalloc/tests.rs at https://github.com/d-sonuga/regalloc2/tree/975dee0ceb56bbc6cbd21554a237babe1e388573.

To resolve this issue, one of the following could be done:

cfallin commented 2 months ago

Indeed, it seems we skip checking operands on branch instructions: link

The stated reason there is that we don't want to check block arguments as normal uses, because we handle them separately (translating to ParallelMove checker instructions); but there's no reason we can't iterate over non-block-arg operands and handle them normally. I'm happy to review a PR for that if you want to try! Or alternately I'm happy to make the change.

Amanieu commented 2 months ago

170 fixes this.

cfallin commented 2 months ago

@Amanieu ah, yes, thanks for that reminder; would you be willing to separate out the checker changes in #170 from the blockparam restrictions and post them in a separate PR?