theotherjimmy opened PR #12319 from theotherjimmy:s390x-z17 to bytecodealliance:main:
Z17 (arch15) includes some instructions that allow us to encode some more complicated operations in fewer instructions. This PR adds support to cranelift-codegen to emit these newer instructions when appropriate.
Further, Z17 includes a VBLEND instruction that mimics the same instruction on x64. Since this is no longer an x64-exclusive instruction type, I've renamed the appropriate stuff within cranelift codegen to reflect that this is not ISA-specific anymore.
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
theotherjimmy requested alexcrichton for a review on PR #12319.
theotherjimmy requested wasmtime-compiler-reviewers for a review on PR #12319.
theotherjimmy updated PR #12319.
theotherjimmy updated PR #12319.
theotherjimmy updated PR #12319.
github-actions[bot] commented on PR #12319:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "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>
alexcrichton commented on PR #12319:
I'm going to shift review of this over to @uweigand
alexcrichton commented on PR #12319:
or, well, I can't officially do that, but @uweigand I'm happy to rubber-stamp once you've approved
uweigand submitted PR review:
Looks mostly good to me, but see inline comments.
In addition to what is implemented here, we now could implement vector integer division for 32-bit and 64-bit integer vectors - but there is currently no ISLE to even express this.
uweigand created PR review comment:
Not sure what if any canonicalization is done at the ISLE level here, but these four don't cover all possible combinations. E.g.
(band x (band (bnot y) z))is not covered.I guess a more fundamental question is which combinations we should be covering. For example, why cover and-not with three inputs but not or-not?
uweigand created PR review comment:
We should also test the encoding for VBLEND and VEVAL here.
uweigand created PR review comment:
We should also add z17 insns to the disassembler, but that is of course a different patch.
uweigand created PR review comment:
I think we need to fix this before committing. (Also, there probably should be run-time test validating the correct behaviour like for other divison operations.)
uweigand created PR review comment:
Should this function also be renamed to lose the
x86then?
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
I stopped because I realized this would add hundreds of rules to get correct, and ran out of steam pretty quickly. I think it's an open question: what should we encode? what 3-input binary operations are actually used?
theotherjimmy updated PR #12319.
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
Done.
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
Added tests.
theotherjimmy updated PR #12319.
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
I made a python script to handle this, and committed the results. My thinking is this:
- This only has to happen once
- It would be better to have the results interspersed in the file near their 2-input counterparts for better
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
Is the integer overflow we're checking for diving
INTMINby-1?
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
That's a yes. I missed that bit of the instruction description.
theotherjimmy updated PR #12319.
theotherjimmy submitted PR review.
theotherjimmy created PR review comment:
Fixed.
uweigand submitted PR review:
This looks all good to me now, except for the
sremimplementation - see inline comment.
uweigand created PR review comment:
The
sdivcase looks good now, but thesremcase is wrong - in the case of division of INT_MIN by -1, only the quotient overflows, not the remainder. The remainder in this case is simply zero, just as with any division by -1. See the comment above and themaybe_avoid_srem_overflowhelper. I think this can be implemented following the same logic for $I128 as well.
Last updated: Jan 29 2026 at 13:25 UTC