T0b1 (Apr 14 2023 at 22:39): T0b1 (Apr 14 2023 at 22:39):Sorry if this is the wrong stream but I don't think there is one for regalloc2(?)
I'm currently hacking a bit on regalloc2 with the current main branch. Now to make the new regalloc trait work with wasmtime I simply removed the is_move function from the trait impl in cranelift.
Now, when running the func::import_works (link) test I get a segfault.
This is because the emit code for the call generates a move for the last call in the test because the sixth argument is a reftype and thus can't be used with an OperandConstraint::Reg for the call but due to the calling conv it needs to be in a reg. However, this move never gets emitted because regalloc2 doesn't know that its a move anymore and cranelift assumes regalloc2 emitted the move.
Now I think my question is, is the move skipping in cranelift incorrect? Removing it seems to do the trick but I don't know if that's gonna break something else. Also, if the register allocator chooses to use the same reg that now emits the move anyway, though that's a small concern.
Jamey Sharp (Apr 14 2023 at 22:46):I don't personally understand enough of RA2 to answer your question, but I just want to say that I think this is a fine stream to ask this question in.
Chris Fallin (Apr 14 2023 at 22:47):Could you add more context on what you're trying to do / what you've done already? You'r switching Cranelift to use a more recent version of RA2?
Chris Fallin (Apr 14 2023 at 22:47):The line you linked (the conditional to skip instructions) should no longer exist, either, since RA2 no longer elides program moves (by CL's definition)
Chris Fallin (Apr 14 2023 at 22:48):with that fix, I suspect/hope that your upgrade works
Chris Fallin (Apr 14 2023 at 22:48):(I don't have much mental bandwidth to put on this right now but if there's a true issue, beyond the above, we'll need to debug it when we next upgrade RA2 in CL. cc @Trevor Elliott as well)
Trevor Elliott (Apr 14 2023 at 23:15):I have a branch where I've updated to a newer version of RA2, but I have only tried running filetests and sightglass benchmarks with it so far.
T0b1 (Apr 14 2023 at 23:18):Chris Fallin said:
Could you add more context on what you're trying to do / what you've done already? You'r switching Cranelift to use a more recent version of RA2?
Yes, I'm trying to prototype a greedy approach to see how much compile time improvements you could (possibly) get (as that is related to my bachelor thesis I finished recently and it's also fun). As such I thought starting with the current main for regalloc2 is better since cranelift will move to it eventually (and simply removing the trait impls for is_move made it compile at least).
I just ran into this problem trying to see if my implementation of the allocator had any problems and was a bit confused about what exactly was the 'incorrect' code in cranelift. The tests seem to pass now so thanks!
Chris Fallin (Apr 15 2023 at 00:12):OK, cool. Let us know if removing the line linked above solves the problem. If not, we can help debug further at some point!
Trevor Elliott (Apr 21 2023 at 20:50):Just to follow up here, I finished upgrading cranelift to regalloc2-0.7.0, so you should be able to rebase on main and have it just work :thumbs_up:
T0b1 (Apr 21 2023 at 22:14):Thanks! I'll check it out
Last updated: Oct 23 2024 at 20:03 UTC