Copyright header question: 8132207: Update for x86 exp in the math lib
Igor Veresov
igor.veresov at oracle.com
Fri Sep 18 07:56:28 UTC 2015
Hi Vivek,
There are some slight build problems with this.
Probably a missling include of macroAssembler_x86.hpp. Try building without precompiled headers (USE_PRECOMPILED_HEADER=0).
/opt/jprt/T/P1/062856.iggy/s/hotspot/src/cpu/x86/vm/macroAssembler_x86_libm.cpp:192:6: error: incomplete type 'MacroAssembler' named in nested name specifier
void MacroAssembler::fast_exp(XMMRegister xmm0, XMMRegister xmm1, XMMRegister xmm2, XMMRegister xmm3, XMMRegister xmm4, XMMRegister xmm5, XMMRegister xmm6, XMMRegister xmm7, Register eax, Register ecx, Register edx, Register tmp) {
^~~~~~~~~~~~~~~~
/opt/jprt/T/P1/062856.iggy/s/hotspot/src/share/vm/asm/assembler.hpp:40:7: note: forward declaration of 'MacroAssembler'
class MacroAssembler;
^
1 error generated.
On non-intel platforms, this is an issue. supports_sse2() here should probably be abstracted away as “should_use_math_stub()” or something to make it more platform independent. Otherwise on other CPUs, supports_sse2() is simply undeclared.
/opt/jprt/T/P1/062856.iggy/s/hotspot/src/share/vm/opto/library_call.cpp
/opt/jprt/T/P1/062856.iggy/s/hotspot/src/share/vm/opto/library_call.cpp: In member function 'bool LibraryCallKit::inline_math_native(vmIntrinsics::ID)':
/opt/jprt/T/P1/062856.iggy/s/hotspot/src/share/vm/opto/library_call.cpp:1765:12: error: 'supports_sse2' is not a member of 'VM_Version'
return VM_Version::supports_sse2() ? runtime_math(OptoRuntime::Math_D_D_Type(), StubRoutines::dexp(), "dexp") :
^
Also, I believe you tested this, but could you please add a unit test (somewhere to hotspot/test/compiler) that would verify that j.l.Math.exp() returns results as intended? Otherwise there's some pretty complicated code there to be proved correct just by eyeballing it.
igor
> On Sep 16, 2015, at 2:54 PM, Deshpande, Vivek R <vivek.r.deshpande at intel.com> wrote:
>
> Hi Igor,
>
> Could you please sponsor this patch. Vladimir and Christian have reviewed the patch.
> Thanks.
>
> Regards,
> Vivek
>
> -----Original Message-----
> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
> Sent: Wednesday, September 16, 2015 2:05 PM
> To: Deshpande, Vivek R
> Cc: Vladimir Kozlov; Viswanathan, Sandhya; hotspot-compiler-dev at openjdk.java.net
> Subject: Re: Copyright header question: 8132207: Update for x86 exp in the math lib
>
> Looks much better. Thanks you.
>
>> On Sep 16, 2015, at 8:10 AM, Deshpande, Vivek R <vivek.r.deshpande at intel.com> wrote:
>>
>> Hi Christian, Please let us know if the updated patch looks good to you.
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Wednesday, September 16, 2015 10:58 AM
>> To: Deshpande, Vivek R; Viswanathan, Sandhya;
>> hotspot-compiler-dev at openjdk.java.net
>> Subject: Re: Copyright header question: 8132207: Update for x86 exp in
>> the math lib
>>
>> Looks good to me.
>>
>> Thanks,
>> Vladimir
>>
>> On 9/16/15 10:51 AM, Deshpande, Vivek R wrote:
>>> Hi Vladimir
>>>
>>> We have updated the patch with your suggestions and with the suggested copyright header.
>>>
>>> Bug-id:
>>> https://bugs.openjdk.java.net/browse/JDK-8132207
>>> webrev:
>>> http://cr.openjdk.java.net/~mcberg/8132207/webrev.03/
>>>
>>> Thanks.
>>>
>>> Regards,
>>> Vivek
>>>
>>> -----Original Message-----
>>> From: Viswanathan, Sandhya
>>> Sent: Friday, September 11, 2015 3:32 PM
>>> To: Vladimir Kozlov; Deshpande, Vivek R; Christian Thalinger
>>> Cc: david.katleman at oracle.com; David Holmes
>>> Subject: RE: Copyright header question: 8132207: Update for x86 exp
>>> in the math lib
>>>
>>> Hi Vladimir,
>>>
>>> Yes we can do that. We will send you the updated patch.
>>>
>>> Thanks,
>>> Sandhya
>>>
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Friday, September 11, 2015 3:00 PM
>>> To: Viswanathan, Sandhya; Deshpande, Vivek R; Christian Thalinger
>>> Cc: david.katleman at oracle.com; David Holmes
>>> Subject: Re: Copyright header question: 8132207: Update for x86 exp
>>> in the math lib
>>>
>>> Can you do as I suggested in previous mail?
>>>
>>> /*
>>> * Copyright (c) 2015, Intel Corporation.
>>> * Intel Math Library (LIBM) Source Code.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or
>>> modify it
>>>
>>> and the rest of standard Oracle template after that.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 9/11/15 2:50 PM, Viswanathan, Sandhya wrote:
>>>> Hi Vladimir,
>>>>
>>>> It has been a week since; please let us know how to proceed on the copyright header.
>>>> Vivek has everything else done and ready to submit the updated patch.
>>>>
>>>> Best Regards,
>>>> Sandhya
>>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Thursday, September 03, 2015 2:25 PM
>>>> To: Viswanathan, Sandhya; Deshpande, Vivek R; Christian Thalinger
>>>> Cc: david.katleman at oracle.com; David Holmes
>>>> Subject: Copyright header question: 8132207: Update for x86 exp in
>>>> the math lib
>>>>
>>>> Hi Davis K.
>>>>
>>>> I hope you can help or advise us about Copyright header in new file
>>>> contributed by Intel:
>>>>
>>>> http://cr.openjdk.java.net/~mcberg/8132207/webrev.01/raw_files/new/s
>>>> r c /cpu/x86/vm/macroAssembler_x86_libm.cpp
>>>>
>>>> Currently it has 2 copyright blocks, Oracle's and Intel's.
>>>> It looks like I am wrong about Oracle's Copyright line since it is
>>>> new file. But format of Intel's header does not match our template.
>>>>
>>>> I can suggest following to have the same form. Or we can just kepp
>>>> Intel's header as it is? What do you think?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> /*
>>>> * Copyright (c) 2015, Intel Corporation.
>>>> * Intel Math Library (LIBM) Source Code.
>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>> *
>>>> * This code is free software; you can redistribute it and/or modify it
>>>> * under the terms of the GNU General Public License version 2 only, as
>>>> * published by the Free Software Foundation.
>>>> *
>>>> * This code is distributed in the hope that it will be useful, but WITHOUT
>>>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>>>> * version 2 for more details (a copy is included in the LICENSE file that
>>>> * accompanied this code).
>>>> *
>>>> * You should have received a copy of the GNU General Public
>>>> License version
>>>> * 2 along with this work; if not, write to the Free Software Foundation,
>>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>> *
>>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>>>> * or visit www.oracle.com if you need additional information or have any
>>>> * questions.
>>>> *
>>>> */
>>>>
>>>>
>>>>
>>>> On 9/3/15 12:15 PM, Viswanathan, Sandhya wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> There are other files which have two copyright notices in the OpenJDK sources. Our LIBM team asked us to include Intel header as a second header in this file.
>>>>>
>>>>> Best Regards,
>>>>> Sandhya
>>>>>
>>>>> -----Original Message-----
>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>> Sent: Thursday, September 03, 2015 11:45 AM
>>>>> To: Deshpande, Vivek R; Christian Thalinger
>>>>> Cc: Viswanathan, Sandhya
>>>>> Subject: Re: RFR (M): 8132207: Update for x86 exp in the math lib
>>>>>
>>>>> On 9/3/15 11:18 AM, Deshpande, Vivek R wrote:
>>>>>> Thanks Vladimir.
>>>>>> Also to confirm, shall I remove the Oracle Copyright ?
>>>>>
>>>>> NO! All Hotspot files have to have it. As I said add Intel's Copyright line at the head of macroAssembler_x86_libm.cpp:
>>>>>
>>>>>>>> /*
>>>>>>>> * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
>>>>>>>> * Copyright (c) 2015, Intel Corporation. All rights reserved.
>>>>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>>
>>>>>
>>>>> I was talking about removing next separate Copyright paragraph you have in the file - you should remove that (and it duplicates information in first official header anyway):
>>>>>
>>>>> 24 /*
>>>>> 25 * Intel Math Library (LIBM) Source Code
>>>>> 26 * Copyright (c) 2015, Intel Corporation.
>>>>> 27 *
>>>>> 28 * This program is free software; you can redistribute it and/or modify it
>>>>> 29 * under the terms and conditions of the GNU General Public License,
>>>>> 30 * version 2, as published by the Free Software Foundation.
>>>>> 31 *
>>>>> 32 * This program is distributed in the hope it will be useful, but WITHOUT
>>>>> 33 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>>> 34 * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>>> 35 * more details.
>>>>> 36 */
>>>>>
>>>>>
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Vivek
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>>> Sent: Thursday, September 03, 2015 11:15 AM
>>>>>> To: Deshpande, Vivek R; Christian Thalinger
>>>>>> Cc: Viswanathan, Sandhya
>>>>>> Subject: Re: RFR (M): 8132207: Update for x86 exp in the math lib
>>>>>>
>>>>>> On 9/3/15 10:58 AM, Deshpande, Vivek R wrote:
>>>>>>> Hi Vladimir
>>>>>>>
>>>>>>> Thank you for your review and comments. I will create a fresh webrev to track on cr.openjdk and update it with your suggestions.
>>>>>>>
>>>>>>> It is 3.75x faster than current FPU code and 3x faster than the sharedRuntime::dexp().
>>>>>>
>>>>>> Very nice!
>>>>>>
>>>>>>> Two fast_exp methods are for 64 bit and 32 bit platforms.
>>>>>>
>>>>>> I missed #ifdef in this big file.
>>>>>>
>>>>>>> We received the assembly code generated by Intel C compiler for LIBM library, so it is hard to add comments for blocks.
>>>>>>
>>>>>> I see.
>>>>>>
>>>>>> * Intel Math Library (LIBM) Source Code
>>>>>>
>>>>>> Add Comment (not in Copyright header) that "code generated by Intel C compiler for LIBM library".
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> Please let me know if you have any suggestions.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Vivek
>>>>>>> -----Original Message-----
>>>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>>>> Sent: Wednesday, September 02, 2015 5:25 PM
>>>>>>> To: Deshpande, Vivek R; Christian Thalinger
>>>>>>> Cc: Viswanathan, Sandhya
>>>>>>> Subject: Re: RFR (M): 8132207: Update for x86 exp in the math lib
>>>>>>>
>>>>>>> And I forgot main question: what is performance improvement vs current FPU code and vs our C code SharedRuntime::dexp() implementations?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 9/2/15 5:16 PM, Vladimir Kozlov wrote:
>>>>>>>> Looks better. I look through all changes and they look reasonable.
>>>>>>>>
>>>>>>>> I would suggest to add few comments inside fast_exp() code to
>>>>>>>> explain what each block of code is doing.
>>>>>>>>
>>>>>>>> Why you have two MacroAssembler::fast_exp() methods?
>>>>>>>>
>>>>>>>> And copyright header should be change since it does not comply
>>>>>>>> to our format (only one line with Intel copyright should be added):
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
>>>>>>>> * Copyright (c) 2015, Intel Corporation. All rights reserved.
>>>>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>>>>> *
>>>>>>>> * This code is free software; you can redistribute it
>>>>>>>> and/or modify it ...
>>>>>>>>
>>>>>>>> Please, create new open webrev on cr.openjdk to track review process.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 8/20/15 3:29 PM, Deshpande, Vivek R wrote:
>>>>>>>>> Hi Vladimir, Christian
>>>>>>>>>
>>>>>>>>> I have updated the patch for Math.exp() using LIBM with the
>>>>>>>>> suggestions mentioned to Sandhya during JVM language summit.
>>>>>>>>>
>>>>>>>>> Please find the patch attached with the mail.
>>>>>>>>>
>>>>>>>>> Let me know your thoughts on the changes and if you have
>>>>>>>>> further suggestions.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>> Vivek
>>>>>>>>>
>>>>>>>>> *From:*Deshpande, Vivek R
>>>>>>>>> *Sent:* Thursday, July 23, 2015 11:01 AM
>>>>>>>>> *To:* 'hotspot-compiler-dev at openjdk.java.net'
>>>>>>>>> *Cc:* Vladimir.Kozlov at oracle.com; Viswanathan, Sandhya
>>>>>>>>> *Subject:* RFR (M): 8132207: Update for x86 exp in the math lib
>>>>>>>>>
>>>>>>>>> Hi all
>>>>>>>>>
>>>>>>>>> I would like to contribute a patch which optimizes Math.exp()
>>>>>>>>> for
>>>>>>>>> 64 and
>>>>>>>>> 32 bit X86 architecture using Intel LIBM implementation.
>>>>>>>>>
>>>>>>>>> Please review and sponsor this patch.
>>>>>>>>>
>>>>>>>>> Bug-id: https://bugs.openjdk.java.net/browse/JDK-8132207
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~mcberg/8132207/webrev.01/
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Vivek
>>>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150918/fd53d52b/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list