Stream: git-wasmtime

Topic: wasmtime / issue #6087 riscv64: Add `Zba` extension instr...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 23 2023 at 18:33):

afonso360 commented on issue #6087:

Thanks for reviewing this! I've also added a bunch more rules to the sextend+arithmetic pattern, those are all in the last commit. Let me know if you'd prefer that split into it's own PR.

Even with all those rules, I think they can still be generalized further. Especially the shifts with const imm, but I noticed that I could also cleanup the regular shift rules in the same way, so I've left those improvements for a future PR.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 23 2023 at 18:55):

afonso360 commented on issue #6087:

I found another issue with the new shift rules, since they can have a i128 RHS we need to explicitly access the value regs. I've updated that commit, but I'm going to run the icache fuzzer to doublecheck that I haven't missed any other cases.

A note for future work: the riscv64 implementation of Imm12::maybe_from_u64 has issues similar to the aarch64 version with assuming that the high bits of an iconst are sign-extended, when we want them to be zero-extended instead. I don't think this is a correctness issue, just the immediate-operand rules may not always match when you expect them to.

:thinking: Yeah, I should look into that.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 23 2023 at 18:56):

afonso360 edited a comment on issue #6087:

I found another issue with the new shift rules, since they can have a i128 RHS we need to explicitly access the value regs. I've updated that commit, but I'm going to run the icache fuzzer for a bit to doublecheck that I haven't missed any other cases.

A note for future work: the riscv64 implementation of Imm12::maybe_from_u64 has issues similar to the aarch64 version with assuming that the high bits of an iconst are sign-extended, when we want them to be zero-extended instead. I don't think this is a correctness issue, just the immediate-operand rules may not always match when you expect them to.

:thinking: Yeah, I should look into that.

Last updated: Oct 23 2024 at 20:03 UTC