Stream: git-wasmtime

Topic: wasmtime / PR #11942 cranelift: ISLE wrapper for construc...

Wasmtime GitHub notifications bot (Oct 26 2025 at 07:25):

efJerryYang opened PR #11942 from efJerryYang:jerrys-fix-for-6038 to bytecodealliance:main:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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.

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. 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.

Wasmtime GitHub notifications bot (Oct 26 2025 at 07:25):

efJerryYang requested alexcrichton for a review on PR #11942.

Wasmtime GitHub notifications bot (Oct 26 2025 at 07:25):

efJerryYang requested wasmtime-compiler-reviewers for a review on PR #11942.

Wasmtime GitHub notifications bot (Oct 26 2025 at 07:28):

efJerryYang edited PR #11942:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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.

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. 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.

Wasmtime GitHub notifications bot (Oct 26 2025 at 08:45):

github-actions[bot] commented on PR #11942:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Oct 26 2025 at 15:40):

efJerryYang edited PR #11942:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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.

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.

Wasmtime GitHub notifications bot (Oct 27 2025 at 14:33):

efJerryYang edited PR #11942:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

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.

Wasmtime GitHub notifications bot (Nov 01 2025 at 17:31):

alexcrichton requested fitzgen for a review on PR #11942.

Wasmtime GitHub notifications bot (Nov 01 2025 at 17:31):

alexcrichton commented on PR #11942:

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)

Wasmtime GitHub notifications bot (Nov 04 2025 at 00:37):

fitzgen created PR review comment:

Here is a simple example of how the updated usage doesn't really seem to be an improvement. We are still passing the unwieldy Imm64 type around and explicitly masking to the relevant type, the only difference is we are doing const instead of iconst.

For this kind of change to be an actual improvement, rather than just moving deck chairs around and being size-of-one and half-a-dozen-of-another, I think we would want to have this rule's RHS be something like this:

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

Where my_const is whatever the new constant constructor for integer types is called.

Specifically, this folds the imm64_masked call into the helper, which gives us two benefits over the current status quo:

1. We are dealing with u64 instead of Imm64, which is what all of the cprop intermediate computations actually use.
2. We don't need to think about type masking as much ourselves nor repeatedly pass ty around in multiple places.

Wasmtime GitHub notifications bot (Nov 04 2025 at 00:37):

fitzgen created PR review comment:

Can we avoid as casts here? Instead, we should use .cast_[un]signed() and {i,u}64::try_from(...).unwrap() as necessary. This makes the behavior we intend explicit.

If we expect that the high bits might be set, and we want to ignore them anyways, then we should explicitly mask them away via Imm64::{zero,sign}_extend_from_width before calling i64::try_from(...).unwrap() and then feeding the bits into Ieee64::with_bits et al.

Otherwise, if the high bits should never be set, then we should just use i64::try_from(...).unwrap() directly.

Wasmtime GitHub notifications bot (Nov 04 2025 at 00:37):

fitzgen created PR review comment:

Probably makes sense to leave these vector rules as they were as well. Not clear to me that splatting is the best default for vectors, or if we should always take a 128-bit value and reinterpret it as the vector type, or even if we should have a default constant constructor for vector types. Either way, I think the explicit splatting in the old version was clearer here.

Wasmtime GitHub notifications bot (Nov 04 2025 at 00:37):

fitzgen submitted PR review:

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.

Wasmtime GitHub notifications bot (Nov 04 2025 at 00:37):

fitzgen created PR review comment:

And all of the float constants can remain as {f32,f64}const usages because the ergonomics are better and it doesn't make sense to me why we would want a single constant constructor for both integers and floats, given that a constructor has a single type signature for all rules.

Wasmtime GitHub notifications bot (Nov 04 2025 at 22:27):

efJerryYang submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2025 at 22:27):

efJerryYang created PR review comment:

Thanks for this comment! I will keep in mind to use the correct cast later when contributing to this repo, but according to what you have mentioned at the very beginning comment, we will not include the changes for float/vec, is that correct?

Wasmtime GitHub notifications bot (Nov 04 2025 at 22:29):

efJerryYang commented on PR #11942:

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.

Wasmtime GitHub notifications bot (Nov 07 2025 at 18:22):

fitzgen commented on PR #11942:

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.

Wasmtime GitHub notifications bot (Nov 07 2025 at 18:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 07 2025 at 18:23):

fitzgen created PR review comment:

we will not include the changes for float/vec, is that correct?

Correct. The review has my thoughts at a couple of different points in time, apologies for that.

Last updated: Dec 06 2025 at 07:03 UTC