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.

Sign up for GitHub

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

Jump to bottom

Zfbfinxmin extension? #27

Closed
asb opened this issue Mar 15, 2023 · 5 comments · Fixed by #31
Closed

Zfbfinxmin extension? #27

asb opened this issue Mar 15, 2023 · 5 comments · Fixed by #31

Comments

@asb
Copy link
Contributor

asb commented Mar 15, 2023

All of the existing floating point extensions have a *inx variant (zfinx, zdinx, zhinx, and zhinxmin). Do you plan to define one for for zfbfinxmin?
@kdockser
Copy link
Collaborator

It would be good to have an zfbfinxmin extension. However, creating such an extension was under the preview of the Zfinx Task Group. Since that group has been dissolved, it is probably up to the Unprivileged Architecture Standing Committee.

@asb asb mentioned this issue Mar 24, 2023
Merged
@asb
Copy link
Contributor Author

asb commented Mar 24, 2023

Thanks for the response Ken. I have zero personal interest in zfbfinxmin, but it does feel like it would be a more efficient use of everyone's time if zfbfinxmin were defined alongside the other extensions, now that (for better or worse) the precedent has been set that floating point extensions should have an *inx variant. It sounds like the alternative might leave such a gap for an extended time, given there's no clear working group to champion the proposal? That said, I've not been directly involved in the current version of the RISC-V ISA standard tracks so perhaps my assessment is wrong there?

I've drafted a PR that adds such an extension. #31

@kdockser
Copy link
Collaborator

Hi Alex. Thanks so much for your enthusiasm!

While I agree with you that such an extension makes sense, I do not believe zfbfinxmin belongs in the BF16 extensions specification, but rather in the zfinx extensions specification where all of the other "inx" extensions reside. Perhaps you could submit a pull request there.

Thanks,
Ken

@asb
Copy link
Contributor Author

asb commented Mar 24, 2023 •
edited
Loading

I don't want to put undue burden on this effort - but as you said, the zfinx extension task group has been dissolved. The zfinx repo is dead as the spec has been merged into the riscv-isa-manual, and it doesn't seem appropriate to submit zfbfinx against riscv-isa-manual.

I do think it would be better if the package of extensions that get proposed for ratification includes zfbfinxmin rather than leaving the gap to be fixed later, and I would have thought having the extension defined within this repo is the only way of achieving that. But if there's no appetite for it, I'm happy enough to leave it.

@kdockser
Copy link
Collaborator

kdockser commented Apr 5, 2023

I'm looking into this.

asb added a commit to llvm/llvm-project that referenced this issue May 19, 2023
@asb
[RISCV][MC] Add support for experimental Zfbfmin extension
35ff5eb 
Provides MC layer support for Zfbfmin: scalar BF16 conversions.

As documented, this extension includes FLH, FSH, FMV.H.X, and FMH.X.H as
defined in Zfh/Zfhmin, but doesn't require either extension.

No Zfbfinxmin has been defined (though you would expect one in the
future, for symmetry with Zfhinxmin). See issue
riscv/riscv-bfloat16#27.

Differential Revision: https://reviews.llvm.org/D147610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add zfbfinxmin extension asb/riscv-bfloat16
2 participants
@asb @kdockser