Re: RFR(M)(round 2): 8215902: Add support for SoftFloat-3e library
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 0x003F is added to 0x0001, the correct result is 0x0040, but the default software floating point implementation returns 0x. 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
Re: RFR(M)(round 2): 8215902: Add support for SoftFloat-3e library
Hi Magnus, thanks for the review! I haven't received a review for the hotspot source changes yet, so I will have to wait. 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 0x003F > > > > is > > > > added to 0x0001, the correct result is 0x0040, but > > > > the > > > > default software floating point implementation returns > > > > 0x. > > > > 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 > > > > > >
Re: RFR(M)(round 2): 8215902: Add support for SoftFloat-3e library
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 0x003F is added to 0x0001, the correct result is 0x0040, but the default software floating point implementation returns 0x. 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
Re: RFR(M)(round 2): 8215902: Add support for SoftFloat-3e library
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 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 0x003F is > >added to 0x0001, the correct result is 0x0040, but the > >default software floating point implementation returns > > 0x. > >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 > > > >
