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

efJerryYang · 2025-10-26T07:25:32Z

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

wasmtime/cranelift/codegen/src/opts/cprop.isle

Lines 227 to 230 in 311bf0f

    
           (rule (simplify (splat (ty_vec128 dst) (iconst $I8 n))) 
        
                 (vconst dst (splat8 (u64_uextend_imm64 $I8 n)))) 
        
           (rule (simplify (splat (ty_vec128 dst) (iconst $I16 n))) 
        
                 (vconst dst (splat16 (u64_uextend_imm64 $I16 n))))

, 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:

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 efJerryYang@d0df113.

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

wasmtime/cranelift/codegen/src/opts/cprop.isle

Lines 233 to 248 in 8e22ff8

    
           (rule (simplify (splat (ty_vec128 dst) (iconst $I64 n))) 
        
                 (vconst dst (splat64 (u64_uextend_imm64 $I64 n)))) 
        
           (rule (simplify (splat (ty_vec128 dst) (f32const _ (u32_from_ieee32 n)))) 
        
                 (vconst dst (splat32 n))) 
        
           (rule (simplify (splat (ty_vec128 dst) (f64const _ (u64_from_ieee64 n)))) 
        
                 (vconst dst (splat64 n))) 
        
           (decl splat8 (u64) Constant) 
        
           (rule (splat8 n) (splat16 (u64_or n (u64_shl n 8)))) 
        
           (decl splat16 (u64) Constant) 
        
           (rule (splat16 n) (splat32 (u64_or n (u64_shl n 16)))) 
        
           (decl splat32 (u64) Constant) 
        
           (rule (splat32 n) (splat64 (u64_or n (u64_shl n 32)))) 
        
           (decl splat64 (u64) Constant) 
        
           (extern constructor splat64 splat64)

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

wasmtime/cranelift/codegen/src/opts/cprop.isle

Line 395 in 311bf0f

(subsume (f16const $F32 r)))

appears to be a typo, so I fixed it ($F32 -> $F16).

As suggested in cranelift: ISLE wrapper for constructing constants #10528 (comment), 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.

…stants

github-actions · 2025-10-26T08:45:52Z

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 · 2025-11-01T17:31:31Z

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)

fitzgen

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.

fitzgen · 2025-11-04T00:14:14Z

cranelift/codegen/src/isle_prelude.rs

+        #[inline]
+        fn ieee16_from_imm64(&mut self, val: Imm64) -> Ieee16 {
+            Ieee16::with_bits(val.bits() as u16)
+        }
+
+        #[inline]
+        fn ieee32_from_imm64(&mut self, val: Imm64) -> Ieee32 {
+            Ieee32::with_bits(val.bits() as u32)
+        }
+
+        #[inline]
+        fn ieee64_from_imm64(&mut self, val: Imm64) -> Ieee64 {
+            Ieee64::with_bits(val.bits() as u64)
+        }
+
+        #[inline]
+        fn imm64_from_ieee16(&mut self, val: Ieee16) -> Imm64 {
+            Imm64::new(val.bits() as i64)
+        }
+
+        #[inline]
+        fn imm64_from_ieee32(&mut self, val: Ieee32) -> Imm64 {
+            Imm64::new(val.bits() as i64)
+        }
+
+        #[inline]
+        fn imm64_from_ieee64(&mut self, val: Ieee64) -> Imm64 {
+            Imm64::new(val.bits() as i64)
+        }


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.

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?

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.

fitzgen · 2025-11-04T00:25:49Z

cranelift/codegen/src/opts/cprop.isle

 (rule (simplify
       (isub (fits_in_64 ty)
             (iconst ty (u64_from_imm64 k1))
             (iconst ty (u64_from_imm64 k2))))
-      (subsume (iconst ty (imm64_masked ty (u64_wrapping_sub k1 k2)))))
+      (subsume (const ty (imm64_masked ty (u64_wrapping_sub k1 k2)))))


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:

We are dealing with u64 instead of Imm64, which is what all of the cprop intermediate computations actually use.

We don't need to think about type masking as much ourselves nor repeatedly pass ty around in multiple places.

fitzgen · 2025-11-04T00:30:15Z

cranelift/codegen/src/opts/cprop.isle

-      (subsume (f32const $F32 r)))
+      (subsume (const $F32 (imm64_from_ieee32 r))))
 (rule (simplify (fadd $F64 (f64const $F64 lhs) (f64const $F64 rhs)))
      (if-let r (f64_add lhs rhs))
-      (subsume (f64const $F64 r)))
+      (subsume (const $F64 (imm64_from_ieee64 r))))

 (rule (simplify (fsub $F32 (f32const $F32 lhs) (f32const $F32 rhs)))
      (if-let r (f32_sub lhs rhs))
-      (subsume (f32const $F32 r)))
+      (subsume (const $F32 (imm64_from_ieee32 r))))
 (rule (simplify (fsub $F64 (f64const $F64 lhs) (f64const $F64 rhs)))
      (if-let r (f64_sub lhs rhs))
-      (subsume (f64const $F64 r)))
+      (subsume (const $F64 (imm64_from_ieee64 r))))

 (rule (simplify (fmul $F32 (f32const $F32 lhs) (f32const $F32 rhs)))
      (if-let r (f32_mul lhs rhs))
-      (subsume (f32const $F32 r)))
+      (subsume (const $F32 (imm64_from_ieee32 r))))
 (rule (simplify (fmul $F64 (f64const $F64 lhs) (f64const $F64 rhs)))
      (if-let r (f64_mul lhs rhs))
-      (subsume (f64const $F64 r)))
+      (subsume (const $F64 (imm64_from_ieee64 r))))

 (rule (simplify (fdiv $F32 (f32const $F32 lhs) (f32const $F32 rhs)))
      (if-let r (f32_div lhs rhs))
-      (subsume (f32const $F32 r)))
+      (subsume (const $F32 (imm64_from_ieee32 r))))
 (rule (simplify (fdiv $F64 (f64const $F64 lhs) (f64const $F64 rhs)))
      (if-let r (f64_div lhs rhs))
-      (subsume (f64const $F64 r)))
+      (subsume (const $F64 (imm64_from_ieee64 r))))


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.

fitzgen · 2025-11-04T00:33:46Z

cranelift/codegen/src/opts/cprop.isle

+      (const dst n))
 (rule (simplify (splat (ty_vec128 dst) (iconst $I16 n)))
-      (vconst dst (splat16 (u64_uextend_imm64 $I16 n))))
+      (const dst n))
 (rule (simplify (splat (ty_vec128 dst) (iconst $I32 n)))
-      (vconst dst (splat32 (u64_uextend_imm64 $I32 n))))
+      (const dst n))
 (rule (simplify (splat (ty_vec128 dst) (iconst $I64 n)))
-      (vconst dst (splat64 (u64_uextend_imm64 $I64 n))))
+      (const dst n))
+(rule (simplify (splat (ty_vec128 dst) (f16const _ (u16_from_ieee16 n))))
+      (const dst (imm64 n)))
 (rule (simplify (splat (ty_vec128 dst) (f32const _ (u32_from_ieee32 n))))
-      (vconst dst (splat32 n)))
+      (const dst (imm64 n)))
 (rule (simplify (splat (ty_vec128 dst) (f64const _ (u64_from_ieee64 n))))
-      (vconst dst (splat64 n)))
+      (const dst (imm64 n)))


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.

efJerryYang · 2025-11-04T22:29:20Z

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 · 2025-11-07T18:22:30Z

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.

Gonzalosilvalde and others added 4 commits October 25, 2025 15:08

cranelift: ISLE wrapper for constructing constants

57c2f25

cranelift: replace iconst to const for RHS rules

522bdee

cranelift: add ieee conversion helpers and rules for float/vector con…

418191b

…stants

cranelift: clean up redundant code and update comments

7da9069

efJerryYang requested a review from a team as a code owner October 26, 2025 07:25

efJerryYang requested review from alexcrichton and removed request for a team October 26, 2025 07:25

github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Oct 26, 2025

alexcrichton requested review from fitzgen and removed request for alexcrichton November 1, 2025 17:31

fitzgen reviewed Nov 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

efJerryYang commented Oct 26, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 26, 2025

Uh oh!

alexcrichton commented Nov 1, 2025

Uh oh!

fitzgen left a comment

Uh oh!

fitzgen Nov 4, 2025

Uh oh!

efJerryYang Nov 4, 2025

Uh oh!

fitzgen Nov 7, 2025

Uh oh!

fitzgen Nov 4, 2025

Uh oh!

fitzgen Nov 4, 2025

Uh oh!

fitzgen Nov 4, 2025

Uh oh!

efJerryYang commented Nov 4, 2025

Uh oh!

fitzgen commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	(rule (simplify (splat (ty_vec128 dst) (iconst $I8 n)))
	(vconst dst (splat8 (u64_uextend_imm64 $I8 n))))
	(rule (simplify (splat (ty_vec128 dst) (iconst $I16 n)))
	(vconst dst (splat16 (u64_uextend_imm64 $I16 n))))

	(rule (simplify (splat (ty_vec128 dst) (iconst $I64 n)))
	(vconst dst (splat64 (u64_uextend_imm64 $I64 n))))
	(rule (simplify (splat (ty_vec128 dst) (f32const _ (u32_from_ieee32 n))))
	(vconst dst (splat32 n)))
	(rule (simplify (splat (ty_vec128 dst) (f64const _ (u64_from_ieee64 n))))
	(vconst dst (splat64 n)))

	(decl splat8 (u64) Constant)
	(rule (splat8 n) (splat16 (u64_or n (u64_shl n 8))))
	(decl splat16 (u64) Constant)
	(rule (splat16 n) (splat32 (u64_or n (u64_shl n 16))))
	(decl splat32 (u64) Constant)
	(rule (splat32 n) (splat64 (u64_or n (u64_shl n 32))))
	(decl splat64 (u64) Constant)
	(extern constructor splat64 splat64)

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

Are you sure you want to change the base?

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

Conversation

efJerryYang commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Notes

Uh oh!

github-actions bot commented Oct 26, 2025

Subscribe to Label Action

Uh oh!

alexcrichton commented Nov 1, 2025

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

fitzgen Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

efJerryYang Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

fitzgen Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

efJerryYang commented Nov 4, 2025

Uh oh!

fitzgen commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

efJerryYang commented Oct 26, 2025 •

edited

Loading