cranelift: ISLE wrapper for constructing constants for integers, floating-points and vectors

[efJerryYang]

Description

This should close #6038.

This PR builds on top of PR #10528, but replaces the ty_vec type with ty_vec128, which is the only one that's actually used in those productions (e.g., https://github.com/bytecodealliance/wasmtime/blob/311bf0f3dcae521a808401148219cf1f16515ee5/cranelift/codegen/src/opts/cprop.isle#L227-L230, and adds float handling on top of the draft in issue #6038. Note, many functions are not pure, we cannot use (decl pure const (Type u64) Value) as by #6038.

Note: f128const support is missing at the moment, will include it if maintainers are happy with this PR.

Testing

• cranelift/filetests/filetests/egraph/cprop-splat.clif - Added %f16x8.
• cranelift/filetests/filetests/egraph/cprop.clif - Added %f16_fneg_const, covering f16const usage.
• cranelift/filetests/filetests/egraph/cprop.clif – Added %u64_to_f64_const, covering the integer-to-float conversion.

All tests pass for cranelift-tools according to the instructions at https://docs.wasmtime.dev/contributing-testing.html

Notes

A few additional clarifications:

1. Using const with float types incurs an extra Ieee64 -> Imm64 -> Ieee64 round trip. This is unavoidable because decl const cannot be overloaded, so it cannot accept const (Type Ieee64) or other float types directly (e.g., Ieee32 or Ieee16). I also tried passing DataValue to allow a generic holder for types like Imm64 and Ieee64, but that likewise caused problems and would require larger changes (the core issue is that Type is a struct rather than an enum; if Type were an enum like DataValue this would be much simpler, but as it stands we can't leverage it directly). After consideration, I'm submitting the Imm64-based version. The discarded DataValue version can be found at https://github.com/efJerryYang/wasmtime/commit/d0df1133c45722df5011ba61d682af0b6bb7724d.
2. I additionally added support for F16, along with test cases. I noticed the previous inconsistency but wasn't sure whether it was intentional or an oversight, see https://github.com/bytecodealliance/wasmtime/blob/8e22ff89f6affe4f79fcebdb416d0ab401d43c97/cranelift/codegen/src/opts/cprop.isle#L233-L248 the Constant rules code below it has F16 but this simplify rule doesn't, and I added the F16 rule production. Adding this rule does not break any existing tests, and I also added test cases for F16. Also, (subsume (f16const $F32 r)) in https://github.com/bytecodealliance/wasmtime/blob/311bf0f3dcae521a808401148219cf1f16515ee5/cranelift/codegen/src/opts/cprop.isle#L395-L395 appears to be a typo, so I fixed it ($F32 -> $F16).
3. As suggested in https://github.com/bytecodealliance/wasmtime/pull/10528#issuecomment-2784280332, i replaced the RHS usages of iconst, vconst, f16const, f32const and f64const to const and ran the cargo test -p cranelift-tools with all test cases pass.

[github-actions[bot]]

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "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.

[alexcrichton]

I don't feel the best to review this so I'm going to move over to @fitzgen, but thanks for the PR @efJerryYang!

(sorry for the delay, we were all at the wasm CG meeting last week)

[efJerryYang]

Thanks for sending this PR!

After looking at these changes, I'm not sure that using the same constructor for all of {ints, floats, vectors} is worth it. Some of the updated usage sites don't really seem like an improvement. It is also strange to have to create float and vector consts from Imm64s and such.

I think it would be better if we had a unified constructor for just ints and didn't try to also unify floats and vector types under that as well. See my comments below.

Thanks for your review on this @fitzgen ! Given that the purpose of #6038 is to unify the usage of different constant types, but we observed the unnecessary complexity in this PR if we include float/vec, do you think our goal will need to be changed to: replace iconst to some general int type or just change the iconst usage to allow handling different ints like you demonstrated here, to avoid working on u64 directly?

(subsume (my_const ty (u64_wrapping_sub k1 k2)))

I will take a closer look soon. And the description of #6038 would then be a bit misleading in this situation.

[fitzgen]

do you think our goal will need to be changed to: replace iconst to some general int type or just change the iconst usage to allow handling different ints like you demonstrated here, to avoid working on u64 directly?

Yes, although I think that iconst itself will remain the same and we will want to define a new helper in practice. This is because we generate the constructors for all CLIF opcodes in build.rs and iconst is one of those, so it is not easy to change in practice. Easier to add a new helper that calls out to iconst under the covers.