elliottt opened PR #6237 from elliottt:trevor/bump-regalloc2
to bytecodealliance:main
:
- Bump RA2 to 0.7.0
- Certify the RA2 update
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former is currently awaiting merge in #6238.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt submitted PR review.
elliottt created PR review comment:
This is a little surprising, but also a totally reasonable consequence of no longer communicating program moves to RA2.
elliottt has marked PR #6237 as ready for review.
elliottt requested wasmtime-compiler-reviewers for a review on PR #6237.
elliottt requested abrown for a review on PR #6237.
elliottt requested wasmtime-default-reviewers for a review on PR #6237.
elliottt requested cfallin for a review on PR #6237.
elliottt updated PR #6237.
elliottt created PR review comment:
This was easily resolved by copying the sret register in
Lower::new
, instead of generating a temporary that we would move into later.
elliottt created PR review comment:
This is a great example of something the redundant move eliminator would have fixed before.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This may be similar to the s390x variant in that something riscv-specific may be needed?
alexcrichton created PR review comment:
I don't know much about the ra2 changes side of things, but if this wasn't already noted it might be worthwhile to look into this perhaps?
alexcrichton created PR review comment:
Random drive-by comment, but this looks like something s390x-specific may be required here?
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
I'm hoping that whatever is causing
mv a0,a0
in the riscv backend is easy to fix.
jameysharp created PR review comment:
Here the VCode pretty-printing changed without changing the disassembly listing. The same instruction was originally printed as
mv a0,a1
and now is printed asmv a1,a0
, but in both cases it's disassembled asori a0, a1, 0
. Any idea what's going on there? I don't see any Rust-side changes that explain this. There are more cases like this below.
jameysharp created PR review comment:
It's neat that this is actually removing moves in some cases compared to the older RA2!
jameysharp created PR review comment:
Similarly, I hope whatever is causing
lgr %r2, %r2
in s390x is easy to fix.
elliottt updated PR #6237.
elliottt created PR review comment:
That's actually due to my change to
sret
register handling :sweat_smile:I'll land it as a separate PR once I've resolved all of the new move issues.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt updated PR #6237.
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former is currently awaiting merge in #6238.This currently has commits from #6245, #6253, and #6252.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former is currently awaiting merge in #6238.This currently has commits from #6253.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #6237.
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former was udpated in #6238.This currently has commits from #6253.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former was updated in #6238.This currently has commits from #6253.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt edited PR #6237:
Bump regalloc2 to version 0.7.0, which removes program moves and adds back in support for scratch registers. This bump also brings with it a new dependency on
hashbrown-0.13.2
andrustc-hash
. The latter had an audit that we could import, while the former was updated in #6238.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #6237.
elliottt created PR review comment:
This pattern shows up across all backends. We insert a copy of the ref-typed values before calling a function with them as an argument, as the values themselves will be constrained to live on the stack.
https://github.com/bytecodealliance/wasmtime/blob/e6339b27254d9e9c4c2fe7bdf1a3a06589c74847/cranelift/codegen/src/machinst/abi.rs#L2243-L2253
With program moves no longer being special for RA2, the move that's inserted by cranelift on line 95 doesn't end up being redundant with the one that RA2 inserts. The store to the stack can use that value without any issue, however we still need to copy the value back tox0
to pass as the first argument tofn0
. Supporting overlap in RA2 will help a lot here, as the value will be allowed to exist on the stack and in a register at the same time, giving us a path forward for skipping the initial copy when setting up the arg.
elliottt has marked PR #6237 as ready for review.
elliottt requested alexcrichton for a review on PR #6237.
elliottt requested jameysharp for a review on PR #6237.
alexcrichton submitted PR review.
jameysharp submitted PR review:
Ooh, I wonder if this will work for me now:
/bench_x64
cfallin submitted PR review:
Updates look good now, diffs are minimal and well-understood -- thanks for the diligence here!
elliottt merged PR #6237.
Last updated: Dec 23 2024 at 13:07 UTC