search

bytecodealliance / regalloc2

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

Simplify parallel-moves state machine #158

Closed jameysharp closed 1 year ago

jameysharp commented 1 year ago

I was trying to understand the public API of ParallelMoves by the method of reading the source, despite the presence of reasonable documentation I could have read instead, and I found some things about the implementation confusing. This PR makes it not confusing to me, which is hopefully a plus for others too?

One thing I'm not clear on: does anybody rely on the output move-list containing self-moves? If you tell the resolver that you want to move, say, rdx into rdx, the generated list of moves will have the same effects whether that self-move is included in the final list or not. But if the caller looks at the generated move list to pick a scratch register, for example, then removing the self-move could make the caller think that rdx is unused. Is that something to actually worry about?

Fuzzing this PR hasn't found any bugs in 50 million iterations, so I at least don't think it's an issue for the way the fuzz target uses this module.

cfallin commented 1 year ago

Re: self-moves: these are a vestige of an earlier, less-smart checker that required "mov self to self but with new vreg" moves as a sort of metadata. We no longer need that so it should be safe to skip these.