bytecodealliance / regalloc2

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

Extend the fuzzer to check the debug locations output #194

Open d-sonuga opened 2 months ago

d-sonuga commented 2 months ago

The checker only checks for the correctness of the allocation dataflow but not for the debug locations output. Although the correctness of the debug locations output isn’t so critical, I think having it checked will still be a good idea, even if it’s just for testing.

cfallin commented 2 months ago

I agree, this would be ideal to check. I don't have time to add it currently; if you want to make an attempt, I'm happy to review though.

d-sonuga commented 2 months ago

Yeah, I’ll do it

Amanieu commented 2 months ago

Note that debug info as currently implemented is only valid within the instructions themselves but not between original instructions. For example parallel move generation may evict a value to get a scratch register, but this isn't currently reflected in the debug info.

d-sonuga commented 1 month ago

@cfallin, @Amanieu, is it correct for there to be multiple entries in the debug_value_labels input with the same label? Because the fuzzer is generating such inputs.

cfallin commented 1 month ago

The same label can be attached to different vregs across different ranges of instruction indices, though these shouldn't overlap (i.e. the label should be in only one place at a time).

d-sonuga commented 1 month ago

Right - so that means that this:

debug_value_labels: [
        (VReg(vreg = 0, class = Int), Inst(2), Inst(5), 53), 
        (VReg(vreg = 4, class = Int), Inst(2), Inst(8), 53), ]

isn't correct, right? Because the ranges overlap.

cfallin commented 1 month ago

That indeed seems invalid! It looks like this loop ensures that over a single instance of the loop, it generates non-contiguous ranges, but it picks an arbitrary label and that label value might have been mentioned in another instance of the loop as well. I guess a fix could be to allocate a new label at that point (though this would prevent labels from moving between vregs, which is interesting to test, but doing better would require some sort of tracking where labels are free).