8210425: [x86] sharedRuntimeTrig/sharedRuntimeTrans compiled without optimization

[Erik Joelsson]

Yes, this looks good, thanks for hanging in there!

/Erik


On 2018-09-14 02:04, Magnus Ihse Bursie wrote:
>
>
> 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
>>
>