Hi Jakub,
On 25/01/2019 9:17 am, Jakub Vaněk wrote:
Hi Magnus,
thanks for the review!
I haven't received a review for the hotspot source changes yet, so I
will have to wait.
Not an expert on the details of the FP code but the wrapper layer
appears okay to me.
One nit with the test is that we don't name tests using bug numbers any
more, so please rename the test to something more descriptive ...
perhaps FloatPrecisionTest.java ?
Also on the test, given this wraps a number of FP functions does the
test need to be more elaborate to ensure they have all been covered?
Thanks,
David
-----
Regards,
Jakub
On 2019-01-23 at 13:55 +0100, Magnus Ihse Bursie wrote:
Hi Jakub,
On 2019-01-15 17:31, Jakub Vaněk wrote:
Hi Magnus and Erik,
I have added the link to the repository to README and I have
removed
the link to the mailing list thread. I have also recreated the
GitHub
repository. Now it is a fork of the mentioned repository with two
extra
commits containing README and the build scripts.
New webrev URL:
http://cr.openjdk.java.net/~jakvanek/8215902/webrev.04/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215902
Sorry for the late reply.
This looks very good! Thank you for fixing this, including rebasing
the
github repo.
I'm not sure if you've gotten reviews from the hotspot team for the
hotspot source changes, but from a build perspective, this is good to
go.
/Magnus
Regards,
Jakub
On 2019-01-15 at 15:05 +0100, Magnus Ihse Bursie wrote:
On 2018-12-25 16:19, Jakub Vaněk wrote:
Hi,
please review this webrev. It is a successor of the softfloat-3
[patch]
thread (first email
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-November/031311.html
)
Changes since the last patch (v6):
- renamed --with-softloat* to --with-sflt* (it is more compact
and
it
corresponds to the old --with-sflt-lib=... option)
- license is now obtained via --with-sflt-license switch (so it
is
not
included in OpenJDK source tree)
- updated documentation (slight rewording, added the license
option)
- checks for default --with/--without behavior are in place
again
(I forgot them when I changed the way the library is
detected)
- added a simple testcase - I found a disrepancy between
softfloat
and
system function behavior. When a float with bits 0x003FFFFF
is
added to 0x00000001, the correct result is 0x00400000, but
the
default software floating point implementation returns
0x00000000.
However I'm not sure where to put this test - now it is in
test/hotspot/jtreg/compiler/floatingpoint.
- comments in code refer to CR 6757269 and newly JDK-8215902
too.
I have created a repository with SoftFloat-3e with build
configuration
specifically for OpenJDK on armel:
https://github.com/ev3dev-lang-java/softfloat-openjdk
I can add a link to it to the documentation.
Bug: https://bugs.openjdk.java.net/browse/JDK-8215902
Webrev: http://cr.openjdk.java.net/~jakvanek/8215902/webrev.02/
Hi Jakub,
In general this looks good.
Some comments:
I agree with Erik that you can add a link to your github project;
compiling SoftFloat is outside the scope of the OpenJDK build
instructions, but it can sure be helpful to lower the bar for
users
wanting to do that. Just one question: any particular reason you
didn't
create your github repo by forking the official
https://github.com/ucb-bar/berkeley-softfloat-3? That way, it
would
have
been easy for users to see that you were not adding any malicious
or
suspicious code to the original SoftFloat distribution.
On the other hand, I think the link to
http://mail.openjdk.java.net/pipermail/aarch32-port-dev/2016-November/000611.html
is unnecessary and just creates clutter in the documentation.
Please
remove it.
/Magnus
CI build:
https://ci.adoptopenjdk.net/view/ev3dev/job/openjdk12_build_ev3_linux/67/
Cheers,
Jakub
