Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zfbfinxmin extension #31

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Add zfbfinxmin extension #31

merged 1 commit into from
Apr 26, 2023

Conversation

asb
Copy link
Contributor

@asb asb commented Mar 24, 2023

With Zfinx, Zdinx, Zhinx, and Zhinxmin there's a clear precedent that floating point extensions should have an analogous "inx" version. As defining such a variant is straight-forward, I feel it would be most sensible (and efficient) to define it as part of the bfloat16 definition process rather than leaving a gap within the ISA to be filled at a later date.

This drafts such a definition. As the bfloat16 extensions are defined in new documentation rather than patches to the riscv-isa-manual, I've duplicated some explanatory text from the Zfinx chapater of the manual so that the zfbfinxmin definition makes sense standalone. If merging into the ISA manual, it would of course take a different form.

Fixes #27.

@asb asb mentioned this pull request Mar 24, 2023
Copy link
Contributor

@nibrunieAtSi5 nibrunieAtSi5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor typos

doc/riscv-bfloat16-zfbfinxmin.adoc Outdated Show resolved Hide resolved
doc/riscv-bfloat16-zfbfinxmin.adoc Outdated Show resolved Hide resolved
With Zfinx, Zdinx, Zhinx, and Zhinxmin there's a clear precedent that
floating point extensions should have an analogous "inx" version. As
defining such a variant is straight-forward, I feel it would be most
sensible (and efficient) to define it as part of the bfloat16 definition
process rather than leaving a gap within the ISA to be filled at a later
date.

This drafts such a definition. As the bfloat16 extensions are defined in
new documentation rather than patches to the riscv-isa-manual, I've
duplicated some explanatory text from the Zfinx chapater of the manual
so that the zfbfinxmin definition makes sense standalone. If merging
into the ISA manual, it would of course take a different form.
@asb
Copy link
Contributor Author

asb commented Mar 24, 2023

Thanks - I've fixed those typos and repushed.

Copy link
Contributor

@nibrunieAtSi5 nibrunieAtSi5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have an opinion on whether this should be integrated as part of this TG effort (I will let that to @kdockser), but the definition of the extension looks sound to me.

|Mnemonic
|Instruction
|FCVT.BF16.S | <<insns-fcvt.bf16.s>>
|FCVT.S.BF16 | <<insns-fcvt.s.bf16>>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would FCVT.S.BF16 be equivalent to SLLI rd, rs1, 16?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does NaN boxing works on Zdinx/Zfinx, does we need to inject 1s into the 32 MSBs in that case ? (which would not be handled by the SLLI if the input was not a proper single precision NaN-boxed float to begin with)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no FP load instructions with Zfinx so there's no guarantee that an input operand is properly nan-boxed for any FP instruction. So the hardware can't check that on inputs. FP instructions do still produce nan-boxed outputs.

So FCVT.BF16.S would not be exactly equivalent to SLLI for the upper bits. But since nan-boxing is already weakened for Zfinx is that important?

@aswaterman
Copy link
Member

there's a clear precedent

Note that Zfa consciously did not define a Zfinx-compatible variant, instead leaving that to future work if demand ever arose. So we have already broken with keeping these things orthogonal. I'd be OK with punting this one, too, but it's not a strong opinion.

@asb
Copy link
Contributor Author

asb commented Apr 26, 2023

there's a clear precedent

Note that Zfa consciously did not define a Zfinx-compatible variant, instead leaving that to future work if demand ever arose. So we have already broken with keeping these things orthogonal. I'd be OK with punting this one, too, but it's not a strong opinion.

Thanks for the important factual correction there. I'd overlooked zfa due to it not yet being ratified.

I don't think leaving it until later would be the worst thing ever - to my mind it depends a lot on what the overhead of spinning up an effort to revisit it at a later point. If that's fairly heavyweight, my preference in general would be that extensions like this (or to be honest zfa) make the minimal changes to ensure uniform support for z*inx (if that is indeed desirable).

Zfa having left *inx for a later date definitely strengthens the argument for doing the same here.

@kdockser kdockser merged commit 263e771 into riscv:main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zfbfinxmin extension?
5 participants