afonso360 requested cfallin for a review on PR #9135.
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.
afonso360 requested wasmtime-compiler-reviewers for a review on PR #9135.
afonso360 requested wasmtime-default-reviewers for a review on PR #9135.
afonso360 updated PR #9135.
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:
- 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>
beetrees submitted PR review:
Thanks for working on this.
beetrees created PR review comment:
Same as with
fli_constant_from_u64
.
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.
beetrees created PR review comment:
fli_constant_from_u64
will currently panic forF16
as support hasn't been implemented inFliConstant::maybe_from_u64
.
beetrees created PR review comment:
I think
vfmv.f.s
andvfmv.s.f
for 16-bit floats requires thezvfh
extension. The specification isn't 100% clear about this, but it's what QEMU requires for it and makes sense extrapolating from thezve*
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.)
afonso360 submitted PR review.
afonso360 created PR review comment:
Yeah, we currently have 8.1.5 in CI and it doesn't recognize these lower case property names.
afonso360 submitted PR review.
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.
afonso360 submitted PR review.
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.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 updated PR #9135.
beetrees submitted PR review.
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
andf64
requires thezve32f
andzve64d
extensions respectively (these extensions then in turn depend on the scalarF
andD
extensions, and are depended on by theV
extension), not just the scalar type being supported.
afonso360 updated PR #9135.
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
afonso360 submitted PR review.
afonso360 edited PR review comment.
alexcrichton submitted PR review:
Thanks for this!
alexcrichton merged PR #9135.
Last updated: Dec 23 2024 at 13:07 UTC