Skip to content

Conversation

@drom
Copy link
Contributor

@drom drom commented Oct 7, 2025

@nadime15
Copy link
Contributor

nadime15 commented Oct 8, 2025

That’s off topic, but it would be nice if additions to the SoftFloat library could be pushed upstream to the official Berkeley SoftFloat repo, so we have a common place to share improvements, report bugs, and keep things consistent.

By the way, there are already functions (most of the bf16 stuff) that exist only in Spike and aren’t part of the official repo.

@aswaterman
Copy link
Collaborator

@nadime15 indeed, that is a topic for another day. (It isn't sustainable for RVIA to keep modifying softfloat at ucb-bar; we'll need to fork it to riscv-software-src. I will take that responsibility, but it'll be a few months before I work on that.)

@aswaterman aswaterman self-requested a review October 8, 2025 20:53
@youngar youngar force-pushed the master branch 6 times, most recently from 4e8633b to 8d3e470 Compare October 9, 2025 21:33
Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

Thanks @drom, this looks good overall. I'll merge after my minor comments are addressed.

@mslijepc mslijepc force-pushed the master branch 2 times, most recently from 1207f90 to a7de9c6 Compare October 10, 2025 21:55
Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

It's getting pretty close. I think we can merge it once my remaining open comments are addressed.

@mslijepc mslijepc force-pushed the master branch 4 times, most recently from c67d924 to 15c8ef7 Compare October 13, 2025 11:41
@aswaterman
Copy link
Collaborator

@mslijepc I think we might accidentally be relying on Zvfh being present when Zvfbfa is present. My concern stems from this:

#define VI_VFP_COMMON \
  VI_VFP_BASE; \
  require((P.VU.vsew == e16 && p->extension_enabled(EXT_ZVFH)) || \
          (P.VU.vsew == e32 && p->get_isa().get_zvf()) || \
          (P.VU.vsew == e64 && p->get_isa().get_zvd())); \

Don't we need to modify that to something like P.VU.vsew == e16 && p->extension_enabled(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFH)?

@mslijepc mslijepc force-pushed the master branch 2 times, most recently from 2e9556c to dc43be0 Compare October 14, 2025 12:48
@mslijepc
Copy link
Collaborator

@aswaterman I agree. I was thinking to use

 #define require_zvfbfa \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFH);

and maybe

 #define require_zvfbfa_zvfhmin \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFHMIN);

there and also in other cases you mentioned. What do you think? Feel free to propose better naming for macros

@mslijepc
Copy link
Collaborator

@aswaterman @youngar I addressed all the comments. Could you please double check if it's all good in PR (except conflicts we have)?

@aswaterman
Copy link
Collaborator

@aswaterman I agree. I was thinking to use

 #define require_zvfbfa \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFH);

and maybe

 #define require_zvfbfa_zvfhmin \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFHMIN);

there and also in other cases you mentioned. What do you think? Feel free to propose better naming for macros

This looks right, but I don't see it in the PR.

@aswaterman
Copy link
Collaborator

@aswaterman I agree. I was thinking to use

 #define require_zvfbfa \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFH);

and maybe

 #define require_zvfbfa_zvfhmin \
-  require(P.VU.altfmt == 0 || p->extension_enabled(EXT_ZVFBFA)); \
+  require_extension(P.VU.altfmt ? EXT_ZVFBFA : EXT_ZVFHMIN);

there and also in other cases you mentioned. What do you think? Feel free to propose better naming for macros

This looks right, but I don't see it in the PR.

To clarify, these do make sense, but only if they are used in the right places, e.g. inside the CHECK32 macros, not in the global scope of an instruction.

@mslijepc mslijepc force-pushed the master branch 6 times, most recently from 5117164 to aedbea5 Compare October 16, 2025 13:16
Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

This looks good to go. Thanks for the hard work, @drom @youngar @mslijepc!

@aswaterman aswaterman merged commit 3bdb768 into riscv-software-src:master Oct 17, 2025
3 checks passed
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.

5 participants