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

[Volker Simonis]

Hi Coleen,

thanks for catching this. For some reason I missed the _NOINLINE_
macro in allocation.{cpp,hpp}.

I've now defined NOINLINE to '__declspec(noinline)' in
globalDefinitions_visCPP.hpp. Everything still compiles fine. The only
difference to the previous code is that "Stack<E, F>::push_segment()"
from stack.inline.hpp is now not inlined on Windows any more. But as I
wrote before, I don't think that makes a difference.

Here's the new webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8151593.v2

Thanks for sponsoring,
Volker

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