8210425: [x86] sharedRuntimeTrig/sharedRuntimeTrans compiled without optimization

[Magnus Ihse Bursie]

On 2018-09-14 10:37, Severin Gehwolf wrote:
> On Thu, 2018-09-13 at 10:39 -0700, Erik Joelsson wrote:
>> On 2018-09-13 03:02, Severin Gehwolf wrote:
>>> Hi Erik,
>>>
>>> On Wed, 2018-09-12 at 10:02 -0700, Erik Joelsson wrote:
>>>> Hello Severin,
>>>>
>>>> In configure, we now set FDLIBM_CFLAGS based on toolchain type and
>>>> capabilities. In JvmOverrideFiles.gmk, the use of it is still
>>>> conditional on Linux. I don't think it should be. We already have the
>>>> conditionals we need.
>>> Thanks for the review. Updated webrev:
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210425/webrev.03/
>>>
>>> I wasn't sure whether the precompiled headers handling for gcc/clang is
>>> right and was reluctant to move it out of the linux conditional. The
>>> assumption on my end is that if headers are compiled with -O3, we can't
>>> used them for any other opt level. It should still all work. Thoughts?
>> Looking at this again, I now realize the whole file has the treatment
>> for these files repeated for each OS, with slight variations. This
>> becomes a clash with the change we are now attempting which is toolchain
>> oriented. I think the easiest way here would be to keep it OS separated
>> for now by reverting to your previous patch.
>>
>> You could consider putting the FDLIBM_CFLAGS conditional block outside
>> the linux block however and apply the "$(FDLIBM_CFLAGS)
>> $(LIBJVM_FDLIBM_COPY_OPT_FLAG)" flags on the lines in the macosx block
>> further down as well. The changes for fdlibm as they are now will apply
>> on macosx since we use clang there, so the jvm change should too.
> OK, done:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210425/webrev.04/
I think this looks reasonable, but let's hear what Erik has to say about 
it also.

/Magnus
>
> More thoughts?
>
> Thanks,
> Severin
>