Stream: git-wasmtime

Topic: wasmtime / PR #11065 Cranelift: Generate integer numeric ...

Wasmtime GitHub notifications bot (Jun 17 2025 at 22:38):

fitzgen opened PR #11065 from fitzgen:isle-generate-numeric-conversions to bytecodealliance:main:

This automatically generates operations and conversions for integer types for use in ISLE.

Supported types are: {i,u}{8,16,32,64,128}

We generate

• Comparisons (eq, ne, lt, lt_eq, gt, gt_eq)
• Arithmetic operations (add, sub, mul, div, neg)
  • These each have checked, wrapping, and unwrapping variants
• Bitwise operations (and, or, xor, shifts, counting leading/trailing zeros/ones)
• A variety of predicates (is_zero, is_power_of_two, is_odd, etc...)
  • These generate both partial constructors and a handful of extractors
• Conversions
  • These come in a variety of flavors: fallible, infallible, truncating, unwrapping, sign-reinterpretation
  • Fallible conversions are also available as an extractor

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

Wasmtime GitHub notifications bot (Jun 17 2025 at 22:38):

fitzgen requested alexcrichton for a review on PR #11065.

Wasmtime GitHub notifications bot (Jun 17 2025 at 22:38):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #11065.

Wasmtime GitHub notifications bot (Jun 17 2025 at 22:53):

abrown commented on PR #11065:

I like this. Side thought: should we add a main.rs to cranelift-codegen-meta that prints out all the paths of the generated files? Having all this generated code is great but sometimes it can be tricky to figure out which generated code in is being used. I've been thinking of doing some refactoring to see if we could get there...

Wasmtime GitHub notifications bot (Jun 17 2025 at 23:24):

fitzgen commented on PR #11065:

sometimes it can be tricky to figure out which generated code in is being used

FWIW if you pass -vv to cargo build (or cargo test or whatever) it should print the outputs of any build scripts it runs.

Wasmtime GitHub notifications bot (Jun 17 2025 at 23:53):

abrown commented on PR #11065:

Yeah, for me wading through cargo build -vv is about as appetizing as wading through the target directory hierarchy to figure out which files are actually being used.

Wasmtime GitHub notifications bot (Jun 18 2025 at 03:09):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "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 (Jun 18 2025 at 14:51):

alexcrichton submitted PR review:

Some bikesheds here and there, along with some minor suggestions, but overall :+1:

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

s/zeros/ones/

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

As a bikeshed what about renaming unwrapping_add to add? That matches Rust more closely and the only difference is "always debug_assertions semantics"

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

s/zeros/ones/

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

Is it concerning that we now have hundreds of helpers, probably generating ~thousands of lines of Rust, and we're compiling those N times, one for each backend?

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

As a bikeshed I find "reinterpret" pretty wordy and overly verbose, and additionally it looks like cast_{signed,unsigned} are actually on stable Rust already, so could this perhaps switch to being i64_cast_unsigned? That makes it a little less wordy and additionally aligns with what one would expect in Rust too

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

While you're here mind changing this by adding a u64_reinterpret_as_i64 before the conversion to a smaller bit-width? I think that's a preexisting missing optimization we have.

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

Bikeshedding this a bit, le matches Rust as well, so maybe le instead of lt_eq?

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

It should be possible to collapse these sequential *_try_into_* calls now if you're up for it

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

I think you can probably tweak this a bit to avoid the truncate since the source, ty_bits I think returns u32 instead of u64

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

This can be cleaned up to using an extractor instead of if-let if you're feeling up to it

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

LIke above these can be cleaned up to using extractors as well

Wasmtime GitHub notifications bot (Jun 18 2025 at 14:51):

alexcrichton created PR review comment:

Now that we have i64_eq these reinterprets I think can be removed?

Wasmtime GitHub notifications bot (Jun 18 2025 at 16:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2025 at 16:00):

fitzgen created PR review comment:

Ideally we would only generate what is actually used, but that seems difficult to track and also annoying to update when you are the first person who needed u8_is_zero, which is part of what this PR is trying to alleviate. Ultimately, I think we end up in a better situation than we were before. At least all the generated code is very simple, doesn't use generics, and hopefully shouldn't stress rustc.

Wasmtime GitHub notifications bot (Jun 18 2025 at 16:02):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2025 at 16:02):

fitzgen created PR review comment:

I don't feel super strongly, but lowering and optimization is a place where we need to be really careful about preserving bits, so I wanted to push us towards consciously choosing the correct operation every time, rather than using the default add operation without thinking. But if you and others still prefer add over unwrapping_add, I can certainly make that change.

Wasmtime GitHub notifications bot (Jun 18 2025 at 16:03):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Jun 18 2025 at 18:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2025 at 18:45):

fitzgen created PR review comment:

I kind of like lt_eq because it isn't ambiguous with "little endian" but happy to change if everyone else prefers le

Wasmtime GitHub notifications bot (Jun 18 2025 at 18:58):

fitzgen updated PR #11065.

Wasmtime GitHub notifications bot (Jun 18 2025 at 22:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2025 at 22:34):

alexcrichton created PR review comment:

This seems perhaps a bit overly complicated, could this be (if-let (i16_from_i64 imm) (u64_cast_signed x)) as the entirety of the conversion?

Wasmtime GitHub notifications bot (Jun 18 2025 at 22:34):

alexcrichton created PR review comment:

Nah that makes sense to me

Wasmtime GitHub notifications bot (Jun 18 2025 at 22:34):

alexcrichton created PR review comment:

Personally I don't consider panics in the backend all that serious and relatively easily weeded out with fuzzing, so I'd prefer to keep aligned with Rust-native methods across these and drop the "unwrapping" prefix. I definitely agree it should have unwrapping semantics though, regardless.

Wasmtime GitHub notifications bot (Jun 18 2025 at 23:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2025 at 23:56):

fitzgen created PR review comment:

I originally had roughly this, but it doesn't work unfortunately, because the imm term is sometimes given u64 values that are not sign extended to the type's width. Basically the upper bits are undefined and elsewhere in the system we do not canonicalize them to being sign extended in practice, even though that would simplify things for us here.

FAIL wasmtime/cranelift/filetests/filetests/isa/pulley64/iconst.clif: compile

Caused by:
    compilation of function on line 22 does not match
    the text expectation

    --- expected
    +++ actual
    @@ -1,10 +1,10 @@
     VCode:
     block0:

    -  xconst8 x0, -1
    +  xconst16 x0, -1
       xconst16 x1, 255
       ret

     Disassembled:
    -xconst8 x0, -1
    +xconst16 x0, -1
     xconst16 x1, 255
     ret

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:00):

alexcrichton created PR review comment:

Alas :(

I'll try to come through and clean this up after the PR lands

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:06):

fitzgen updated PR #11065.

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:06):

fitzgen created PR review comment:

Okay, I renamed u64_unwrapping_add to u64_add, etc...

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:08):

fitzgen has enabled auto merge for PR #11065.

Wasmtime GitHub notifications bot (Jun 19 2025 at 00:46):

fitzgen merged PR #11065.

Last updated: Dec 06 2025 at 06:05 UTC