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.
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.
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: Dec 23 2024 at 12:05 UTC