riscv64: Improve ctz/clz/cls codegen

[afonso360]

👋 Hey,

This PR started by fixing the lowering rules for ctz. The base rule had (if-let $false (has_zbb)), but the rule for i8/i16 depended on both Zbb and Zbs. Which meant that if we only had Zbb we had no valid rule for lowering i8/i16. The fuzzer found this on #5844 (comment).

I also ended up improving codegen for these instructions in a number of cases. We try to avoid the base case as much as possible since it involves a loop.

We can now avoid requiring Zbs in all cases by falling back to an ori or or instruction instead to set a single bit.

It also moves some constructors around and other misc stuff since we have a bunch of rules all over the files. I'm trying to collect the ones I find at the top of the file.

It's been fuzzing for about an hour, I'll update this if it finds anything.

[jameysharp]

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.

[afonso360]

No worries, and thanks for looking into it (and all the other PR's)! ❤️

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?

[jameysharp]

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.

[afonso360]

Rebased this on top of #5984, I've also separated the commits a bit, so it should be easier to review commit-by-commit.

[jameysharp]

This was much easier to review, thank you! I have some questions but I think this is in good shape overall.

[jameysharp]

I want to note that the uses of fits_in_64 in the "Restrict lowering rules for ctz/clz" commit should be harmless but shouldn't be necessary. Since the rule for $I128 has higher priority, and since ctz/clz/cls accept only integer types, the only types which could reach the general rule are I8-I64.

It's definitely good to be consistent across rules for whether we're explicit about checking fits_in_64 or not. And it's probably good to be explicit since it serves as a kind of documentation. Currently, doing this causes ISLE to generate slightly worse code, but it's probably not measurably worse, and I hope to fix it eventually anyway.

So you don't need to do anything about this comment. I just wanted to mention that this change shouldn't change any lowering behavior.

[jameysharp]

The immediate here being 16 when I knew the constant needed to be 2^16 confused me so much. But of course 16 is 2^4, and lui shifts its immediate operand left by 12 bits, and 4+12 is 16, so this is correct.

[jameysharp]

I don't understand this TODO comment. As far as I understand we can't use not on the input without checking if it was negative. But isn't that exactly what the rule for lower_cls already does?

[afonso360]

Oops, I think I messed up slightly here. I looked at rust's output for u32::leading_ones, but that has slightly different semantics. LLVM's output for the equivalent operation is still better than our lowering.

example::cls_u64:
        bgez    a0, .LBB4_2
        not     a0, a0
.LBB4_2:
        clz     a0, a0
        addi    a0, a0, -1
        ret

We are doing essentially the same but replacing the branch with select_reg. I'm not sure this can easly be improved in ISLE without being able to emit branches.

Thanks for catching this! I'm going to remove the TODO, we have a bunch of places where replacing select_reg with custom branches would improve codegen, I guess this is just one more of them, so no need for a special mention.

[jameysharp]

Possible future work: Maybe make MInst::Select accept an IntCC etc, so it can replace MInst::SelectReg, and so this lowering doesn't have to generate two separate branches for the same condition.

[jameysharp]

I love it. 👍