Stream: git-wasmtime

Topic: wasmtime / issue #5854 riscv64: Improve `ctz`/`clz`/`cls`...


view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip 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: Dec 23 2024 at 12:05 UTC