RFR (M): 8144748: Move assembler/macroAssembler inline function definitions to corresponding inline.hpp files

[Vladimir Kozlov]

Good.

Thanks,
Vladimir

On 12/4/15 2:27 PM, Mikael Vidstedt wrote:
>
> New webrev:
> http://cr.openjdk.java.net/~mikael/webrevs/8144748/webrev.01/webrev/
>
> Changes since webrev.00:
>
> * ret/retl implementation is now always inline (no #ifdefs) and defined
> in macroAssembler_sparc.inline.hpp, removed from macroAssembler_sparc.cpp
> * Formatting of mov and mov_or_nop updated
>
> Cheers,
> Mikael
>
> On 2015-12-04 13:02, Vladimir Kozlov wrote:
>> > Btw, does anybody know why ret & retl are only inlined in PRODUCT
>> builds?
>>
>> TraceJumps is develop flag which is const 'false' in product (and in
>> optimized too). As result the code of ret() is only one instruction.
>>
>> I think they put code in .cpp because wanted to set breakpoints in
>> these methods in debug VM but debug VM does not inline anyway. I think
>> it should be only in .hpp. Please, remove ifdef and code's copy in .cpp.
>>
>> Also use code stile with {} and separate lines for:
>>
>> + inline void MacroAssembler::mov( Register s,  Register d) {
>> +   if ( s != d )    or3( G0, s, d);
>> +   else             assert_not_delayed();  // Put something useful in
>> the delay slot!
>> + }
>> +
>> + inline void MacroAssembler::mov_or_nop( Register s,  Register d) {
>> +   if ( s != d )    or3( G0, s, d);
>> +   else             nop();
>> + }
>>
>> Thanks,
>> Vladimir
>>
>> On 12/4/15 12:23 PM, Mikael Vidstedt wrote:
>>>
>>> Please review this change which moves a large-ish number of function
>>> definitions/bodies from assembler_sparc.hpp and macroAssembler_sparc.hpp
>>> to the corresponding assembler_sparc.inline.hpp and
>>> macroAssembler_sparc.inline.hpp files.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8144748
>>> Webrev:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8144748/webrev.00/webrev/
>>>
>>>
>>> * Background
>>>
>>> The specific problem which triggered this change was the following
>>> pattern in assembler_sparc.hpp:
>>>
>>> class Assembler : ... {
>>> ...
>>>     inline void emit_int32(int);
>>>     inline void emit_data(int x) { emit_data(x); }
>>> ...
>>> }
>>>
>>> If assembler_sparc.hpp is ever included directly without including
>>> assembler_sparc.inline.hpp this will lead to a use without definition,
>>> since emit_int32 is only defined in assembler_sparc.inline.hpp. The same
>>> pattern is true for almost all of the inline functions in
>>> assembler_sparc/macroAssembler_sparc.
>>>
>>> In general, inline functions (apart from trivial ones) should be defined
>>> in the inline.hpp file and any .cpp file actually making use of them
>>> should include the inline.hpp file instead. In this specific case, for
>>> whatever reason, it seems to be working well with Solaris Studio, but
>>> GCC is generating an error.
>>>
>>>
>>> * About the change
>>>
>>> The change here is very mechanical:
>>>
>>> for every relevant function F:
>>> * add "inline" keyword if needed
>>> * copy function to inline.hpp - trying to place it in the Right
>>> Place(tm)
>>> * add class prefix (Assembler:: or MacroAssembler::)
>>> * remove potential default parameter values
>>> * remove function body from .hpp file
>>>
>>> In a few cases I took the liberty of updating the indentation where it
>>> seemed off.
>>>
>>> Btw, does anybody know why ret & retl are only inlined in PRODUCT
>>> builds?
>>>
>>> Cheers,
>>> Mikael
>>>
>