x64: Mask shift amounts for small types

[afonso360]

This was reported by @bjorn3 in https://github.com/bjorn3/rustc_codegen_cranelift/pull/1268#issuecomment-1223916783 when we tried to remove the explicit shift amount masking done by cg_clif.

As a consequence of this fix, #4699 is also fixed, which is nice!

This PR also does some other housekeeping:

• Enables i128 shifts in the fuzzer
• Enables the i128-shifts-small-types.clif runtests and moves them to i128-shifts.clif
• Adds run tests that trigger the issue above (which is similar, but not the same as the i128 one)
• Adds compile tests for all forms of ishl/sshr/ushr

Fixes: #4699
cc: @elliottt

[github-actions]

Subscribe to Label Action

cc @cfallin, @fitzgen

Details

This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[elliottt]

This looks great to me!

[afonso360]

I think this is also going to invalidate all the fuzzer issues that were reported today, since it changes the input format for the fuzzer, not sure how good OSS-Fuzz is with these situations.

[jameysharp]

You're right, Afonso. Could you remove the function-generator changes from this PR for now while we sort out all those issues?

[jameysharp]

The rustfmt CI failure looks like a transient network issue. Let's re-run it after the rest of the jobs complete.

[jameysharp]

I want to see if I understand why this PR fixes this issue. Could you confirm or correct my interpretation?

It looks like there are two bugs in put_masked_in_imm8_gpr, and I gather that these bugs aren't present in the similar-looking ISLE rules.

If the shift amount is not defined by an iconst instruction, then this function doesn't mask it at all...? The only thing it does is, if the shift amount needs two registers because it's an i128, then it only takes the lower 64-bit register.

If it is a constant, then it's masked by (1 << ty.bits()) - 1. So for an 8-bit LHS, the shift amount is used modulo 256, but it's actually supposed to be modulo 8. For a 64-bit LHS the shift amount is effectively not masked at all. By contrast, the shift_mask function just returns ty.lane_bits() - 1 so it gets this right. (And also handles vector types correctly, I guess?)

I guess the reason these bugs weren't obvious sooner is that wasm only uses i32 and i64 types, and x86 already masks 32-bit and 64-bit shift amounts to 5 and 6 bits, respectively. But narrower 8-bit and 16-bit shifts are also masked to 5 bits on x86, so those were wrong.

Now that I think I understand this, I'm wondering how hard it is in ISLE to avoid emitting the and instruction when the type is i32 or i64.

[afonso360]

If the shift amount is not defined by an iconst instruction, then this function doesn't mask it at all...?

Yes, the source of the issue I was trying to fix was exactly this.

It looks like there are two bugs in put_masked_in_imm8_gpr, and I gather that these bugs aren't present in the similar-looking ISLE rules.

Wow! I was just trying to fix the unmasked non const one, didn't actually realize that the const case didn't do the right thing either!

We should probably add some shift_imm test cases as well.

Now that I think I understand this, I'm wondering how hard it is in ISLE to avoid emitting the and instruction when the type is i32 or i64.

I think that's a good improvement, i'll push it soon.

[afonso360]

And it turns out the const case was also wrong. Great Catch!

[afonso360]

const_to_type_masked_imm8 was also doing the wrong thing, and is now fixed, I've also removed the special cases for rotl/rotr which were using const_to_type_masked_imm8.

We now use the general case that is using put_masked_in_imm8_gpr and doing the right thing for both cases.

These were the only uses of const_to_type_masked_imm8 outside of put_masked_in_imm8_gpr

[jameysharp]

Very nice work! Good catch on noticing that const_to_type_masked_imm8 had the same bug. I'm pleased to see so many of those and instructions disappear from the filetests. And it's great to see the ISLE rules and supporting Rust get both simpler and more correct at the same time!