RFR(S): 8151593: Cleanup definition/usage of INLINE/NOINLINE macros and add xlC support

[Volker Simonis]

Thanks for checking Thomas!

Volker

On Fri, Mar 11, 2016 at 2:30 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Volker, Coleen,
>
> I did a quick test on windows, __declspec(noinline) works for non-member
> functions too and prevents inlining.
>
> Kind regards, Thomas
>
> On Fri, Mar 11, 2016 at 2:08 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com> wrote:
>>
>>
>> This is a nice cleanup.  I'll sponsor it but I have a question.
>>
>> There's some directives to not inline in allocation.hpp that are needed by
>> NMT:
>>
>> // noinline attribute
>> #ifdef _WINDOWS
>>   #define _NOINLINE_  __declspec(noinline)
>> #else
>>   #if __GNUC__ < 3    // gcc 2.x does not support noinline attribute
>>     #define _NOINLINE_
>>   #else
>>     #define _NOINLINE_ __attribute__ ((noinline))
>>   #endif
>> #endif
>>
>>
>> But in your changes:
>>
>> +// Inlining support
>> +// MSVC has '__declspec(noinline)' but it only applies to member
>> functions
>> +#define NOINLINE
>> +#define ALWAYSINLINE __forceinline
>>
>>
>> Would there be an error if you used __declspec(noinline) on non-member
>> functions, so that you can use this directive and extend the cleanup to
>> allocation.hpp ?
>>
>> Thanks,
>> Coleen
>>
>>
>>
>> On 3/11/16 7:41 AM, David Holmes wrote:
>>>
>>> On 11/03/2016 7:45 PM, Volker Simonis wrote:
>>>>
>>>> On Fri, Mar 11, 2016 at 10:36 AM, Mikael Gerdin
>>>> <mikael.gerdin at oracle.com> wrote:
>>>>>
>>>>> Volker,
>>>>>
>>>>> On 2016-03-11 10:31, Volker Simonis wrote:
>>>>>>
>>>>>>
>>>>>> Hi Dan,
>>>>>>
>>>>>> thanks for the hint. I've linked the two issues together in JBS and I
>>>>>> can close the second one as duplicate once the fix is committed.
>>>>>>
>>>>>> How can I use two bug IDs in a changeset? Should I just put it in an
>>>>>> additional "Summary:" line?
>>>>>
>>>>>
>>>>>
>>>>> The changeset format actually allows multiple bug ids in a single
>>>>> commit,
>>>>> the format is:
>>>>>
>>>>> XXXXXXX: Bug title 1
>>>>> YYYYYYY: Bug title 2
>>>>> Summary: foo
>>>>> Reviewed-by: bar
>>>>>
>>>>> See
>>>>> http://openjdk.java.net/guide/producingChangeset.html
>>>>>
>>>>
>>>> Ah, I see. Thanks for the info.
>>>> Nevertheless I'd prefer to use one single bug only and close the other
>>>> as duplicate.
>>>>
>>>> I personally think multiple bug IDs are confusing (and they break our
>>>> internal integration scripts :)
>>>
>>>
>>> Agreed. If there are two bugs for the same issue then one should be
>>> closed as a duplicate and only a single bugid used.
>>>
>>> It would be extremely rare where one changeset needed to fix two
>>> completely independent bugs IMHO.
>>>
>>> David
>>>
>>>> Regards,
>>>> Volker
>>>>
>>>>> /Mikael
>>>>>
>>>>>
>>>>>>
>>>>>> As regards to content, I really think the annotations aren't needed
>>>>>> any more. I've tried with gcc4.1.2 which is the oldest we are using it
>>>>>> it works perfectly.
>>>>>>
>>>>>> Regards,
>>>>>> Volker
>>>>>>
>>>>>> On Thu, Mar 10, 2016 at 6:56 PM, Daniel D. Daugherty
>>>>>> <daniel.daugherty at oracle.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Volker,
>>>>>>>
>>>>>>> The objectMonitor.cpp and synchronizer.cpp NOINLINE issue is tracked
>>>>>>> via:
>>>>>>>
>>>>>>> JDK-8049103 investigate whether inlining still needs to be inhibited
>>>>>>> in
>>>>>>>               objectMonitor.cpp and synchronizer.cpp
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049103
>>>>>>>
>>>>>>> Feel free use both bug IDs in your changeset.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 3/10/16 10:10 AM, Volker Simonis wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> can I please have a review and a sponsor for the following clean-up
>>>>>>>> change:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8151593/
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8151593
>>>>>>>>
>>>>>>>> First I only wanted to implement the INLINE macro for xlC in
>>>>>>>> instanceKlass.inline.hpp. But then I realized that we also define
>>>>>>>> NOINLINE in various other files so I decided to do it "the right
>>>>>>>> way"
>>>>>>>> and clean up the code a little bit.
>>>>>>>>
>>>>>>>> I've now moved the definition of the INLINE/NOINLINE macros to
>>>>>>>> globalDefinitions_<compiler>.hpp. In the case where there is no
>>>>>>>> compiler-specific definition, globalDefinitions.hpp defines empty
>>>>>>>> defaults for the macros.
>>>>>>>>
>>>>>>>> I've also renamed INLINE to ALWAYSINLINE to match its intention more
>>>>>>>> clearly.
>>>>>>>>
>>>>>>>> Currently NOINLINE is really only defined on Linux. This is the same
>>>>>>>> behavior we had before the change. Following some more details:
>>>>>>>>
>>>>>>>> os_linux.cpp
>>>>>>>>
>>>>>>>> - removed the handling for gcc < 3 (even we at SAP don't use this
>>>>>>>> anymore
>>>>>>>> :)
>>>>>>>>
>>>>>>>> objectMonitor.cpp, synchronizer.cpp
>>>>>>>>
>>>>>>>> - completely removed the annotation with NOINLNE which is there
>>>>>>>> since
>>>>>>>> pre-OpenJDK times already. The comment mentions that this was
>>>>>>>> required
>>>>>>>> to prevent build-time failures with 'older' versions of GCC. But we
>>>>>>>> already build without 'NOINLINE' on Linux/ppc64 since years and I've
>>>>>>>> also tested that it still works on Linux/x86.
>>>>>>>>
>>>>>>>> I've Build and smoke-tested on Linux/amd64 and Linux/ppc64,
>>>>>>>> Solaris/SPARC, MacOS X, AIX and Windows.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Volker
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>
>