On Fri, 2018-09-07 at 09:12 -0700, Erik Joelsson wrote: > On 2018-09-07 01:56, David Holmes wrote: > > Hi Severin, > > > > On 7/09/2018 6:01 PM, Severin Gehwolf wrote: > > > Hi, > > > > > > On Thu, 2018-09-06 at 10:55 -0700, Erik Joelsson wrote: > > > > On 2018-09-06 10:29, Severin Gehwolf wrote: > > > > > On Thu, 2018-09-06 at 09:55 -0700, Erik Joelsson wrote: > > > > > Thanks, Erik. GCC supports -ffp-contract since 4.6. Clang has -ffp- > > > > > contract too. Question is beginning from which version. That's why I'd > > > > > expect for those flags to work on linux. Is there anything else I need > > > > > to check? > > > > > > > > > > Would it be preferred if I moved this into a block like this? > > > > > > > > > > ifeq ($(TOOLCHAIN_TYPE), gcc) > > > > > [...] > > > > > endif > > > > > > > > Yes, making it conditional on toolchain type is what David was after. > > > > > > Right. David, given that -ffp-contract is available for clang too, do > > > you still want me to add this into a toolchain specific block? > > > > Personally I'd prefer it but I will defer to Erik and Magnus on this. > > > > To me it sounds like we want this flag if the toolchain is either gcc or > clang, so please make it conditional on that.
Updated webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.02/ It now only sets FDLIBM_CFLAGS if gcc supports it and builds fdlibm without opt if not. Otherwise it uses -O3 and -ffp-contract=off. If the toolchain is clang and on linux it always uses -O3 and -ffp- contract=off. The reason, I've done this in configure is a potential follow-up fix for hotspot via JDK-8210425 where the CFLAGS could get re-used. Thoughts? Thanks, Severin > /Erik > > Thanks, > > David > > > > > Also note that solaris doesn't seem to be handling this code any > > > special, so there would be a difference between gcc + solaris and > > > solstudio + solaris. Rather hypothetical case, I suppose, but still ;-) > > > One of the arguments were that gcc is being used on other platforms > > > than linux. > > > > > > > You can also consider adding a capability check if you know that > > > > versions of the compiler that should still work don't have the feature. > > > > > > One clarification: Does a capability check mean doing this at the > > > configure step or is there a way one has access to toolchain versions > > > in make files? > > > > > > Thanks, > > > Severin > > > > > > > /Erik > > > > > Thanks, > > > > > Severin > > > > > > > > > > > /Erik > > > > > > > Thanks, > > > > > > > Severin > > > > > > > > > > > > > > > Thanks, > > > > > > > > David > > > > > > > > > > > > > > > > On 5/09/2018 11:12 PM, Severin Gehwolf wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > Cross-posting this review-thread on core-libs-dev and > > > > > > > > > build-dev as > > > > > > > > > this > > > > > > > > > is a build change, but affects fdlibm which is core-libs. > > > > > > > > > > > > > > > > > > With JDK-8170153 optimization for fdlibm code has been turned > > > > > > > > > on > > > > > > > > > for > > > > > > > > > ppc64 s390 and aarch64. This patch intends to turn it on on > > > > > > > > > all > > > > > > > > > arches > > > > > > > > > on Linux. I've not observed any precision issues. Is there a > > > > > > > > > good > > > > > > > > > reason to not use -O3 -ffp-contract=off everywhere? > > > > > > > > > > > > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210416 > > > > > > > > > webrev: > > > > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Testing: - java/lang/Math, java/lang/StrictMath tests (all > > > > > > > > > pass). > > > > > > > > > - Currently running through submit repo. > > > > > > > > > > > > > > > > > > A simple micro benchmark from JDK-8170153[1] shows these > > > > > > > > > numbers > > > > > > > > > for > > > > > > > > > StrictMath: > > > > > > > > > > > > > > > > > > Function | before | after > > > > > > > > > ---------------------------------------------- > > > > > > > > > sin | 0m33.382s | 0m18.731s > > > > > > > > > cos | 0m31.562s | 0m18.796s > > > > > > > > > tan | 0m33.657s | 0m21.093s > > > > > > > > > atan | 0m5.714s | 0m4.902s > > > > > > > > > log | 0m6.212s | 0m4.439s > > > > > > > > > log10 | 0m7.946s | 0m5.543s > > > > > > > > > sqrt | 0m0.481s | 0m0.449s > > > > > > > > > cbrt | 0m5.295s | 0m5.214s > > > > > > > > > tanh | 0m1.404s | 0m1.307s > > > > > > > > > log1p | 0m6.457s | 0m5.131s > > > > > > > > > IEEEremainder | 0m10.629s | 0m6.048s > > > > > > > > > atan2 | 0m8.037s | 0m5.668s > > > > > > > > > hypot | 0m2.171s | 0m2.147s > > > > > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Severin > > > > > > > > > > > > > > > > > > > > > > > > > > > >
