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

[Mikael Vidstedt]

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