Stream: git-wasmtime

Topic: wasmtime / PR #9135 riscv64: Initial support for FP16 math

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 12:02):

afonso360 requested cfallin for a review on PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 12:02):

afonso360 opened PR #9135 from afonso360:riscv-zfh to bytecodealliance:main:

:wave: Hey,

This PR adds initial support for FP16 data types in the RISC-V backend. Most of the groundwork was done in #9084 and #9079 which allows us to support a lot of instructions without doing any work.

This PR only really adds encodings for flh/fsh (Load and Store instructions), otherwise all normal floating point instructions support this new format using the existing width field present in all of them.

I think we're only missing fpromote/fdemote and conversions instructions for complete support. This PR also does not add SIMD F16 support, which is supported but under a different ISA flag.

cc: @beetrees since you've also been working on FP16 support in some other backends.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 12:02):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 12:02):

afonso360 requested wasmtime-default-reviewers for a review on PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 12:08):

afonso360 updated PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 13:44):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:riscv64", "cranelift:meta", "isle"

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

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 16:44):

beetrees submitted PR review:

Thanks for working on this.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 16:44):

beetrees created PR review comment:

Same as with fli_constant_from_u64.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 16:44):

beetrees created PR review comment:

    "qemu": "qemu-riscv64 -cpu rv64,v=true,vlen=256,vext_spec=v1.0,zfa=true,zfh=true,zba=true,zbb=true,zbc=true,zbs=true,zbkb=true,zcb=true,x-zicond=true -L /usr/riscv64-linux-gnu",

QEMU has deprecated using capital 'Z' at the start of RISC-V CPU properties.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 16:44):

beetrees created PR review comment:

fli_constant_from_u64 will currently panic for F16 as support hasn't been implemented in FliConstant::maybe_from_u64.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 16:44):

beetrees created PR review comment:

I think vfmv.f.s and vfmv.s.f for 16-bit floats requires the zvfh extension. The specification isn't 100% clear about this, but it's what QEMU requires for it and makes sense extrapolating from the zve* extensions.

(As a side note that doesn't need to be fixed in this PR, similar issues exist for the other vector bitcasts, as support for the zve*/v extensions that define what EEWs are supported is never checked, just that the minimum vector register size is large enough to hold the type.)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:06):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:06):

afonso360 created PR review comment:

Yeah, we currently have 8.1.5 in CI and it doesn't recognize these lower case property names.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:12):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:12):

afonso360 created PR review comment:

Right, I was hoping to avoid dealing with that in this initial version, so I'm going to restrict fli to 32/64 bits and come back to it in a future PR.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:17):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:17):

afonso360 created PR review comment:

I found some discussion about this in https://github.com/riscv/riscv-v-spec/issues/429 which
added the following note to the spec:

... Vector instructions where any floating-point vector operand’s EEW is not a supported floating-point type width (which includes when FLEN < SEW) are reserved.

So we should probably also check the scalar flags for vectors.

(As a side note that doesn't need to be fixed in this PR, similar issues exist for the other vector bitcasts, as support for the zve*/v extensions that define what EEWs are supported is never checked, just that the minimum vector register size is large enough to hold the type.)

Yeah, I don't think we currently do any EEW checks, but with https://github.com/bytecodealliance/wasmtime/pull/9079 it should hopefully be easier now.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:17):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:17):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 17:39):

afonso360 updated PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 18:35):

beetrees submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2024 at 18:35):

beetrees created PR review comment:

This doesn't need to be done in this PR (and the change here is an improvement over the status quo) but vector support for f32 and f64 requires the zve32f and zve64d extensions respectively (these extensions then in turn depend on the scalar F and D extensions, and are depended on by the V extension), not just the scalar type being supported.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2024 at 08:42):

afonso360 updated PR #9135.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2024 at 08:44):

afonso360 created PR review comment:

Right, we don't have great support for all of the embedded extensions, I've been mostly thinking of this backend in requiring at least the V extension. Though it shouldn't be hard to add checks for those

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2024 at 08:44):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2024 at 08:44):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 14:47):

alexcrichton submitted PR review:

Thanks for this!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2024 at 15:03):

alexcrichton merged PR #9135.

Last updated: Nov 22 2024 at 17:03 UTC