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

[David Holmes]

Hi Zhengyu!

On 5/05/2016 11:18 PM, Zhengyu Gu wrote:
>
> 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"

But if that were the case we should not have needed to add "inline" to 
make this change compile.

Cheers,
David
-----

> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>