-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8318158: RISC-V: implement roundD/roundF intrinsics #16382
Conversation
👋 Welcome back omikhaltcova! A progress list of the required criteria for merging this PR into |
@omikhaltsova The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
5a22e5c
to
d8524ff
Compare
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On wording, RoundingMode::rne
says "round to Nearest, ties to Even", while Math.round(float)
says "round to Neares, ties to positive infinity". Are these equivalent? Do we have a test covering that?
I doubt that. Check the result for all x in float, x < 0 && abs(x) < 0x1.0p23f |
Yes, you are both right, this is incorrect implementation. I compared the output of the assembler instructions fcvt.w.s/fcvt.l.d and Java Math.round(), paying attention to the range mentioned above. The results are different. Thank you for pointing me out this mistake! |
/contributor add @VladimirKempik |
@omikhaltsova |
gentle ping, please take a look at this pr! |
Can some reviewer take a look again please ? |
Unfortunately, I witnessed performance regression on sifive unmatched board. Before:
After:
|
@RealFYang I've reproduced this performance regression on VisionFive 2. The results are as follow:
|
That is, to say the very least, surprising. I'd use -prof:perfasm to find out why. |
-prof:perfasm doesn't work on u74 boards(hifive and visionfive2) as is, some problems with cycles event. |
It is. We should not simply accept something like this without trying to understand the reason. |
I think usage of static rounding mode here is fine for riscv.
-- from |
Looks good. |
@Hamlin-Li I've just left some comments. Is this what was required? Could you take a look, please?! |
Thanks for updating. Maybe more comments about the trick of BTW, some minor comments:
|
@Hamlin-Li Thanks for your advices! Fixed. IMHO this will be enough, otherwise, for clearlier understanding, a specific example should be given in the comments showing that the rounding ties to positive infinity is equal to calling sequentially of fadd with RDN and fcvt with RDN. |
For normal cases, I guess |
@Hamlin-Li No, that’s wrong: RUP of riscv won’t work, it’ll give incorrect results. Look at the rounding of some values below for example:
|
fadd_s requires setting the explicit rounding mode RDN (round down towards −∞) because adding 0.5f to some floats exceeds the precision limits for a float and therefore rounding takes place. This leads to the incorrect results in case of the default rounding mode RNE (round to nearest, ties to even) for some inputs:
Let’s consider two of them with RNE for fadd.s:
if RDN is set for fadd.s then:
|
In addition, some examples with RDN for fadd.s and fcvt.w.s:
|
@Hamlin-Li @RealFYang Could you take a look once again, please, whether these comments are sufficient or something else is needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating.
In fact by java api spec, the corner cases also include Integer/Long.MIN/MAX_VALUE
Can you add some comments like below?
It also works for -2.1474836E9f which is corresponding to -2147483648 (Integer.MIN_VALUE) and even less float value;
it also works for 2.1474836E9f which is corresponding to 2147483647 (Integer.MAX_VALUE) and even greater float value;
BTW, some minor comments, I think you mean java.lang.Math
or j.l.Math
instead of java.math
.
Otherwise it looks good to me.
@Hamlin-Li thank you for reviewing! You suggested to write comments similar to aarch64 #16382 (comment). IMHO
|
Just FYI, there is a For special cases,
|
Thank you all very much for the review! /integrate |
@omikhaltsova |
/sponsor |
Going to push as commit 19147f3.
Your commit was automatically rebased without conflicts. |
@VladimirKempik @omikhaltsova Pushed as commit 19147f3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please, review this Implementation of the roundD/roundF intrinsics for RISC-V platform.
In the table below it is shown that NaN argument should be processed as a special case.
The benchmark running with the 2nd fixed implementation on the T-Head RVB-ICE board shows the following performance improvement::
Before
After
Progress
Issue
Reviewers
Contributors
<vkempik@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16382/head:pull/16382
$ git checkout pull/16382
Update a local copy of the PR:
$ git checkout pull/16382
$ git pull https://git.openjdk.org/jdk.git pull/16382/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16382
View PR using the GUI difftool:
$ git pr show -t 16382
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16382.diff
Webrev
Link to Webrev Comment