PING: RFR: JDK-8153743: AllocateHeap() and ReallocateHeap() should use ALWAYSINLINE macro

[Zhengyu Gu]

On 05/04/2016 10:09 PM, David Holmes wrote:
> On 4/05/2016 11:17 PM, Yasumasa Suenaga wrote:
>> Hi David,
>>
>>> We also require:
>>>
>>> diff -r fc5b64f70199
>>> src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>
>> Thanks!
>> I added it to new webrev:
>>
>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.01/
>>
>>>> I'm running it through our build system.
>>
>> If this patch passes JPRT, I think this patch is ready to merge.
>
> It did pass jprt. I am wondering though, why the existing uses of 
> ALWAYSINLINE in instanceKLass.inline.hpp compile fine on Solaris 
> without needing the addition of "inline" to the ALWAYSINLINE macro? 
> All those uses appear in template definitions so perhaps that 
> influences things somehow? But this suggests to me our inlining 
> attempts may not actually be working as we expect.
>
According to Solaris Studio User's Guide 
(https://docs.oracle.com/cd/E24457_01/html/E21991/gljol.html)
always_inline

    Equivalent to#pragma inlineand-xinline

sounds like "always_inline" implies "inline"

Change looks good to me.

-Zhengyu

> A second review of this is still needed anyway, so I've added Kim to 
> the cc list for his compiler expertise.
>
> Thanks,
> David
>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2016/05/04 19:16, David Holmes wrote:
>>> On 4/05/2016 12:10 PM, David Holmes wrote:
>>>> On 25/04/2016 10:11 PM, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> Could you review and sponsor it?
>>>>> I confirmed this patch can apply to jdk9/hs repos.
>>>>>
>>>>> However, I checked this patch with GCC on Linux.
>>>>> I cannot check with other compiler.
>>>>
>>>> I'm running it through our build system.
>>>
>>> We also require:
>>>
>>> diff -r fc5b64f70199
>>> src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>> --- a/src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>> +++ b/src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>> @@ -279,6 +279,6 @@
>>>
>>>  // Inlining support
>>>  #define NOINLINE
>>> -#define ALWAYSINLINE __attribute__((always_inline))
>>> +#define ALWAYSINLINE inline __attribute__((always_inline))
>>>
>>>  #endif // SHARE_VM_UTILITIES_GLOBALDEFINITIONS_SPARCWORKS_HPP
>>>
>>> David
>>>
>>>
>>>>> Also other compilers might need "inline" statement in ALWAYSINLINE
>>>>> macro.
>>>>> So I want to hear whether this patch can work with other compilers.
>>>>
>>>> I don't have any input on this - sorry.
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/04/16 1:10, Daniel D. Daugherty wrote:
>>>>>> JDK9-hs and JDK9-hs-rt are currently locked down due to the merge.
>>>>>> Please see the "Merging jdk9/hs-rt with jdk9/hs" e-mail thread on
>>>>>> the hotspot-dev at openjdk.java.net alias.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/15/16 7:28 AM, Yasumasa Suenaga wrote:
>>>>>>> PING: Could you review and sponsor it?
>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.00/
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2016/04/07 23:13, Yasumasa Suenaga wrote:
>>>>>>>> For force inlining, JDK-8076212 uses always_inline attribute to
>>>>>>>> them.
>>>>>>>> JDK-8151593 added ALWAYSINLINE macro for force inlining.
>>>>>>>>
>>>>>>>> For consistency, and for other compiler support, AllocateHeap() 
>>>>>>>> and
>>>>>>>> ReallocateHeap() should use ALWAYSINLINE.
>>>>>>>>
>>>>>>>> I used ALWAYSINLINE to them, but I got error message as below;
>>>>>>>> ------------------------------
>>>>>>>> Building target 'images' in configuration
>>>>>>>> 'linux-x86_64-normal-server-release'
>>>>>>>> In file included from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/utilities/array.hpp:29:0, 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                   from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/memory/universe.hpp:29, 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                   from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/code/oopRecorder.hpp:28, 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                   from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/asm/codeBuffer.hpp:28, 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                   from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/asm/assembler.hpp:28, 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                   from
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/precompiled/precompiled.hpp:29: 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/memory/allocation.inline.hpp:72:20: 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> エラー: always_inline function might not be inlinable
>>>>>>>> [-Werror=attributes]
>>>>>>>>   ALWAYSINLINE char* ReallocateHeap(char *old, size_t size, 
>>>>>>>> MEMFLAGS
>>>>>>>> flag,
>>>>>>>>                      ^
>>>>>>>> ------------------------------
>>>>>>>> * This message includes Japanese because my environment is 
>>>>>>>> Japanese
>>>>>>>> locale :-)
>>>>>>>>
>>>>>>>> According to GCC manual [1], non-static inline function is always
>>>>>>>> compiled
>>>>>>>> on its own in the usual fashion.
>>>>>>>> However, we can compile as inline function with "inline" and
>>>>>>>> "always_inline".
>>>>>>>> always_inline attribute does not imply inlining [2].
>>>>>>>> GCC testcase [3] uses both "inline" and "always_inline" actually.
>>>>>>>>
>>>>>>>> I uploaded webrev. Could you review it?
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.00/
>>>>>>>>
>>>>>>>> I cannot access JPRT.
>>>>>>>> So I need a sponsor.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Inline.html
>>>>>>>> [2] https://chromiumcodereview.appspot.com/14820003/
>>>>>>>> [3]
>>>>>>>> https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/testsuite/gcc.dg/vmx/gcc-bug-i.c?view=markup&pathrev=178730 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>