Stream: git-wasmtime

Topic: wasmtime / PR #9798 aarch64: support udiv for 32bit integers

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:23):

MarinPostma opened PR #9798 from MarinPostma:i32-div-aarch64 to bytecodealliance:main:

This PR takes first steps towards completing #9766, by implementing support for 32bit unsigned division in cranelift and winch.
we had to take the following steps to enable that:

• extend the ISLE lowering rules in cranelift to properly handle 32bits udiv, by have a rule that matches 64bit division, and another for anything else that fits 32bit. The 32 bits basically is the old 64bits rule but for 32bits.
• stop extending registers to 64bits in the ISLE rules.
• patch the emit code to emit the correct instruction for 32bits division.
• in winch: stop emitting extend operations for urem and udiv.

question: we emit a trapz to catch overflow, but trapz currently only work with 64bits registers. We either need a new trapz instruction for 32bits, or, pass the type info to trapz during lower, for emission. This would cover the case where the lower half of the divisor is 0, but not the upper part. Although, is that even possible in practice, by code generated by cranelift?

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:23):

MarinPostma requested cfallin for a review on PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:23):

MarinPostma requested wasmtime-compiler-reviewers for a review on PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:23):

MarinPostma requested alexcrichton for a review on PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:24):

MarinPostma requested wasmtime-core-reviewers for a review on PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:25):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 01:31):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 03:03):

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

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "winch"

Thus the following users have been cc'd because of the following labels:

• saulecabrera: winch

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 (Dec 12 2024 at 15:12):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:13):

MarinPostma edited PR #9798:

This PR takes first steps towards completing #9766, by implementing support for 32bit unsigned division in cranelift and winch.
we had to take the following steps to enable that:

• extend the ISLE lowering rules in cranelift to properly handle 32bits udiv, by have a rule that matches 64bit division, and another for anything else that fits 32bit. The 32 bits basically is the old 64bits rule but for 32bits.
• stop extending registers to 64bits in the ISLE rules.
• patch the emit code to emit the correct instruction for 32bits division.
• in winch: stop emitting extend operations for urem and udiv.
• Make trap_if size aware: this avoids having to perform extensions before cbz, shaving more instructions.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:19):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:23):

MarinPostma submitted PR review.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:23):

MarinPostma created PR review comment:

I think we mostly had two choices here:

• either make the size part of the CondBrKind, which is tighter, but yields more changes in the code
• or setting the size in the op itself (current approach), which required less changes

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:25):

MarinPostma edited PR #9798:

This PR takes first steps towards completing #9766, by implementing support for 32bit unsigned division in cranelift and winch.
we had to take the following steps to enable that:

• extend the ISLE lowering rules in cranelift to properly handle 32bits udiv, by have a rule that matches 64bit division, and another for anything else that fits 32bit. The 32 bits basically is the old 64bits rule but for 32bits.
• stop extending registers to 64bits in the ISLE rules.
• patch the emit code to emit the correct instruction for 32bits division.
• in winch: stop emitting extend operations for urem and udiv.
• Make trap_if size aware: this avoids having to perform extensions before cbz, shaving more instructions. This is reflected in the lowering rules for checking division by zero for udiv.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:25):

MarinPostma has marked PR #9798 as ready for review.

Wasmtime GitHub notifications bot (Dec 12 2024 at 15:28):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:21):

cfallin submitted PR review:

Thanks! One thought below on the way that TrapIf is extended, but generally I like how this is turning out.

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:21):

cfallin created PR review comment:

todo!() here -- can we just show the reg (and below for vector too) with show_reg for now?

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:21):

cfallin created PR review comment:

Also, separately, if we modify CondBrKind, this is toward the direction that we want for supporting 32-bit zero/nonzero tests in regular conditional branches too. We don't have to implement that now (you can unimplemented!() it in the CondBr case; but if you wanted to, it would certainly be fine with me) but it's nice to lay the groundwork.

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:21):

cfallin created PR review comment:

I think I might prefer the first option actually -- I was a bit confused at first how the OperandSize would affect the TrapIf if we're just talking about conditional branch kinds like b.lo, b.hi, etc; until I realized that CondBrKind::Zero(reg) and NonZero(reg) exist and encode cbz/cbnz. If you don't mind, do you think you could try the refactor out? On the upside, it seems like it should get rid of a bunch of extraneous OperandSizes on branches emitted in various places that are condcode-based (e.g. in abi.rs above and in many of the emit-tests below).

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:31):

MarinPostma commented on PR #9798:

Thanks @cfallin, will do the refactor :+1:

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:32):

MarinPostma submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2024 at 23:32):

MarinPostma created PR review comment:

This make sense to me, I'll refactor

Wasmtime GitHub notifications bot (Dec 15 2024 at 09:12):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 15 2024 at 09:23):

MarinPostma updated PR #9798.

Wasmtime GitHub notifications bot (Dec 15 2024 at 10:45):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift:area:machinst", "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 (Dec 17 2024 at 17:04):

MarinPostma requested cfallin for a review on PR #9798.

Wasmtime GitHub notifications bot (Dec 17 2024 at 18:47):

cfallin submitted PR review:

Looks great; thanks a bunch!

Wasmtime GitHub notifications bot (Dec 17 2024 at 19:08):

cfallin merged PR #9798.

Last updated: Dec 23 2024 at 12:05 UTC