fix: panic on aarch64 when combining bitcasting with vector operations

[LaoLittle]

Fixes #12696

Desc

This PR fixes a panic bug in cranelift-codegen.
In current implementation, iadd instruction in lower.isle matches all 64-bit or smaller types and picks a GPR-based iadd for them.

(rule iadd_base_case -1 (lower (has_type (fits_in_64 ty) (iadd x y)))
      (add ty  x y))

We use (ty_int ty) to ensure that only integer values are accepted, and all vector values go to the vector version of iadd.

;; vectors

(rule -2 (lower (has_type ty @ (multi_lane _ _) (iadd x y)))
      (add_vec x y (vector_size ty)))

Changes

lower.isle now matches the iadd for scalar rules for scalar values only when the input value is a scalar.
Added cranelift/filetests/filetests/runtests/issue-12696.clif for regression test.

[cfallin]

Thanks!

However I note that iadd is not the only case affected by the "fits_in_64 == scalar type" confusion -- would you mind auditing for other such cases? E.g. isub is equally affected, I think.

Also, I think that ty_int_ref_scalar_64 is probably a little more idiomatic than combining ty_int and fits_in_64.

[LaoLittle]

Thanks!

However I note that iadd is not the only case affected by the "fits_in_64 == scalar type" confusion -- would you mind auditing for other such cases? E.g. isub is equally affected, I think.

Also, I think that ty_int_ref_scalar_64 is probably a little more idiomatic than combining ty_int and fits_in_64.

Hi, if I use ty_int_ref_scalar_64, the tests will produce new errors such as

thread 'worker #3' (1426398) panicked at /Users/laolittle/RustProjects/wasmtime/target/debug/build/cranelift-codegen-42e62e29c0321dac/out/isle_aarch64.rs:3664:5:
internal error: entered unreachable code: no rule matched for term operand_size at src/isa/aarch64/inst.isle line 1294; should it be partial?

So I will keep this to (fits_in_64 (ty_int)) for now.

[cfallin]

Hi, if I use ty_int_ref_scalar_64, the tests will produce new errors such as

That's pretty surprising: it should be equivalent to the composition of the two matchers you used, and triggering other errors is indicative of something we should fix, not run away from.

It looks like the definition is about what I would expect, except that maybe it's missing a check for !is_dynamic_vector -- would you mind adding that as well and trying again?

[LaoLittle]

Hi, if I use ty_int_ref_scalar_64, the tests will produce new errors such as

That's pretty surprising: it should be equivalent to the composition of the two matchers you used, and triggering other errors is indicative of something we should fix, not run away from.

It looks like the definition is about what I would expect, except that maybe it's missing a check for !is_dynamic_vector -- would you mind adding that as well and trying again?

Thanks for clarifying! I've patched the ty_int_ref_scalar_64 and it works without errors across all of the clif tests.

[cfallin]

Thanks!