Wasmtime GitHub notifications bot (Feb 27 2023 at 23:56): Wasmtime GitHub notifications bot (Feb 27 2023 at 23:56):jameysharp commented on issue #5854:
I've tried to review this a couple of times and have not succeeded yet. I think it would help me if you can split this into smaller PRs. It looks like you already structured individual commits carefully, but I think the main thing I want in this case is to keep separate the movement of definitions from the actual changes in lowering.
I'm open to alternative suggestions, but would you mind opening a new PR that moves existing definitions without changing them first? I'm hoping that makes the overall diff for _this_ PR easier to review as it should only have the actual logic changes.
If that's too much trouble, I'm happy to discuss other ways to split this, or I can try to review it as-is, or we can see if @elliottt or @cfallin is up for reviewing it. My trouble is that since I'm not familiar with risc-v I have to figure that out at the same time as following the motion of existing code and finding the real changes, and together that's apparently a bit too much for my brain.
Wasmtime GitHub notifications bot (Feb 28 2023 at 11:38):afonso360 commented on issue #5854:
No worries, and thanks for looking into it (and all the other PR's)! :heart:️
I'll split this into general RISC-V rules cleanups and move the codegen changes into separate PR's.
On that note, I've been thinking about doing a pass on the entire RISCV ruleset to add helpers for generating instructions.
We have a lot of:
(alu_rr_imm12 (AluOPRRI.Andi) val (imm12_const 255))Which I would really like for it to become something along these lines:
(rv_andi val (imm12 255))I think this would improve readability somwhat, the rules on RISCV are really verbose and I think this is part of it. What do you guys think about that?
Wasmtime GitHub notifications bot (Feb 28 2023 at 13:06):jameysharp commented on issue #5854:
I've heard other people say they consider that kind of helper really important in the x86 rules, so I imagine it's a good idea for all backends. I haven't authored enough backend rules to have an opinion myself though. My first reaction had originally been that it seemed like a lot of work with an unclear benefit, but I'm coming around to the conclusion that it's worth doing after all.
Wasmtime GitHub notifications bot (Mar 13 2023 at 12:52):afonso360 commented on issue #5854:
Rebased this on top of #5984, I've also separated the commits a bit, so it should be easier to review commit-by-commit.
Last updated: Nov 22 2024 at 16:03 UTC