Wasmtime GitHub notifications bot (Oct 10 2023 at 10:26): Wasmtime GitHub notifications bot (Oct 10 2023 at 10:26):alexcrichton requested afonso360 for a review on PR #7203.
Wasmtime GitHub notifications bot (Oct 10 2023 at 10:26):alexcrichton opened PR #7203 from alexcrichton:rv64-icmp to bytecodealliance:main:
This PR is another stab towards improving
icmpalong the lines of https://github.com/bytecodealliance/wasmtime/pull/6112 and https://github.com/bytecodealliance/wasmtime/pull/7113. This tries to draw on both of those PRs a bit to end up with something that's usable by the majority of the backend for use with branches.I've attempted to break things up piecemeal here so the commits in theory are understandable commit-by-commit. This additionally doesn't implement every single optimization I could think of, mostly just a few that seemed to be the low-hanging fruit ones.
Wasmtime GitHub notifications bot (Oct 10 2023 at 10:26):alexcrichton requested cfallin for a review on PR #7203.
Wasmtime GitHub notifications bot (Oct 10 2023 at 10:26):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #7203.
Wasmtime GitHub notifications bot (Oct 10 2023 at 11:51):alexcrichton updated PR #7203.
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:17):afonso360 submitted PR review:
This is very nice! Thank you! Having the commits separated neatly really helped review this.
It all looks correct to me. But I'm not entirely sure I didn't miss anything, so a second pair of eyes would be helpful!
I'm also going to run the fuzzer on this for a few days to check if anything comes up. (Although I don't think that should prevent merging this)
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:17):afonso360 submitted PR review:
This is very nice! Thank you! Having the commits separated neatly really helped review this.
It all looks correct to me. But I'm not entirely sure I didn't miss anything, so a second pair of eyes would be helpful!
I'm also going to run the fuzzer on this for a few days to check if anything comes up. (Although I don't think that should prevent merging this)
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:17):afonso360 created PR review comment:
I don't think this is entirely correct, but it probably doesn't matter.
Due to #6922 we don't generate all 32bit constants from
lui+addiso there are some that fallback to the general load from const pool path.That path uses the
ldinstruction to load the full 8 bytes, but we never sign extend that value while lowering (I think). So there is a chance that in some of the 32bit constants that we miss we may not sign extend them.I've built a small test to check which constants fail to generate, and the lowest one is
0x7FFF_F801so all the negatives are covered. So I don't think there's anything wrong here.It might be worth ensuring that the value is sign extended before emitting it to the constant pool anyway. Or switching to
lwthat only loads 4 bytes and sign extends the load result. Just to make sure that this doesn't happen in the future.
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:18):afonso360 submitted PR review:
This is very nice! Thank you! :heart: Having the commits separated neatly really helped review this.
It all looks correct to me. But I'm not entirely sure I didn't miss anything, so a second pair of eyes would be helpful!
I'm also going to run the fuzzer on this for a few days to check if anything comes up. (Although I don't think that should prevent merging this)
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:27):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Oct 10 2023 at 15:28):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Oct 11 2023 at 06:15):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 11 2023 at 06:15):alexcrichton created PR review comment:
Thanks for the good eye here, I'll dig into this a bit more and possibly omit this now that there's more constant-related optimizations for comparisons anyway. I'll probably leave this out for an initial attempt.
Wasmtime GitHub notifications bot (Oct 12 2023 at 06:06):alexcrichton updated PR #7203.
Wasmtime GitHub notifications bot (Oct 12 2023 at 06:17):alexcrichton updated PR #7203.
Wasmtime GitHub notifications bot (Oct 12 2023 at 06:18):alexcrichton updated PR #7203.
Wasmtime GitHub notifications bot (Oct 14 2023 at 16:16):alexcrichton updated PR #7203.
Wasmtime GitHub notifications bot (Oct 16 2023 at 12:40):afonso360 merged PR #7203.
Last updated: Nov 22 2024 at 16:03 UTC