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

[Coleen Phillimore]

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