Stream: git-wasmtime

Topic: wasmtime / PR #11043 Add lowering rules for `{add,sub,or,...

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

fitzgen requested abrown for a review on PR #11043.

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

fitzgen opened PR #11043 from fitzgen:x64-op-mem-imm 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
-->

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

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

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

fitzgen requested alexcrichton for a review on PR #11043.

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

fitzgen requested wasmtime-core-reviewers for a review on PR #11043.

Wasmtime GitHub notifications bot (Jun 14 2025 at 00:44):

fitzgen updated PR #11043.

Wasmtime GitHub notifications bot (Jun 14 2025 at 01:23):

fitzgen updated PR #11043.

Wasmtime GitHub notifications bot (Jun 14 2025 at 21:09):

alexcrichton submitted PR review:

What do you think about blocking on the suggestion below to clean up lower.isle?

Also other instructions that might be interesting to add are xor (probably easy) and shifts (maybe harder? unsure)

Wasmtime GitHub notifications bot (Jun 14 2025 at 21:09):

alexcrichton created PR review comment:

A possible alternative: what about:

(decl x64_or_mem (Amode Value) SideEffectNoResult)

The type could be inferred from the value, and that would also enable dispatching internally based on iconst-or-not to all the various rules. That way we additionally wouldn't need any new rules in lower.isle and would reduce the duplication there.

Wasmtime GitHub notifications bot (Jun 16 2025 at 16:15):

fitzgen updated PR #11043.

Wasmtime GitHub notifications bot (Jun 16 2025 at 16:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 16 2025 at 16:15):

fitzgen created PR review comment:

Done! Please take another look

Wasmtime GitHub notifications bot (Jun 16 2025 at 17:36):

alexcrichton created PR review comment:

This could replace i64_from_iconst with i32_from_iconst to avoid the if-let

Wasmtime GitHub notifications bot (Jun 16 2025 at 17:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 16 2025 at 17:36):

alexcrichton created PR review comment:

Personally I'm always wary to discard the lower bits with as in lowering rules lest they accidentally get removed. Given that iconst extraction is generally signed could the above rules do (i16_from_iconst imm) and then the immediate to the instruction is (i16_as_u16 imm) to be clear that there's no data loss it's just reinterpreting the sign bit?

Wasmtime GitHub notifications bot (Jun 16 2025 at 17:36):

alexcrichton created PR review comment:

Would it make sense to add i8_from_iconst to make these two cases one-liners too?

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

We can't do that because i32_from_const will unconditionally truncate upper bits, but we need to only match this rule when the iconst bits will losslessly fit in an i32.

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

Similarly, this also needs to be fallible

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

alexcrichton created PR review comment:

I don't think that's correct? The definition of i32_from_iconst uses try_from? (it very much should or we have a lot of incorrect lowering rules)

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

alexcrichton submitted PR review.

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

There is no i16_from_iconst and this is only used to implement u16_from_iconst and our existing foo_from_iconst extractors (such as u32_from_iconst) already use lossy as conversions and bring the assumption that the caller is choosing a value based on the correct width for their type context.

I don't think that is really the correct choice, but I also don't want to do something different and make it so we have multiple idioms here. We already have enough trouble from some things being extractors and some being pure partial constructors and not being able to reuse one kind of thing for the other. All of our iconst and numerics stuff could definitely be overhauled but, again, I don't really want to do it in this PR. I'd rather just follow the existing code conventions.

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

i32_from_iconst seems to do the checks but then we have these other cases that do not:

(decl u32_from_iconst (u32) Value)
(extractor (u32_from_iconst x)
           (u64_from_iconst (u64_as_u32 x)))

;; ...

(decl i8_from_iconst (i8) Value)
(extractor (i8_from_iconst x)
           (i32_from_iconst (i32_as_i8 x)))

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

So I guess this particular one can change, but I don't think we can blanket apply this reasoning to the other cases

Wasmtime GitHub notifications bot (Jun 16 2025 at 19:12):

alexcrichton commented on PR #11043:

Extracting a comment out here since it's not particularly relevant in isolation: I suppose this is fine to land and while on one hand it's not making things worse on the other hand it's also making things worse because it's adding more places to update with a future refactoring. I'll start working on such a refactoring as this is something that's bothered me for quite some time. If you'd like to land this in the meantime I think that's ok too.

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

fitzgen commented on PR #11043:

I was thinking it would be nice to automatically generate the full combinatorial set of fallible, infallible, checked, and truncating ISLE conversions for numeric types in the meta crate

Wasmtime GitHub notifications bot (Jun 16 2025 at 19:17):

fitzgen commented on PR #11043:

But yeah, I'll land this now and then we can do the much needed clean up in follow ups.

Wasmtime GitHub notifications bot (Jun 16 2025 at 19:36):

fitzgen updated PR #11043.

Wasmtime GitHub notifications bot (Jun 16 2025 at 19:36):

fitzgen has enabled auto merge for PR #11043.

Wasmtime GitHub notifications bot (Jun 16 2025 at 20:11):

fitzgen merged PR #11043.

Last updated: Dec 06 2025 at 06:05 UTC