RFR (XL): 6934604: enable parts of EliminateAutoBox by default

[Vladimir Kozlov]

Thank you, Christian

On 5/6/13 12:20 PM, Christian Thalinger wrote:
>
> On Apr 30, 2013, at 7:46 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> http://cr.openjdk.java.net/~kvn/6934604.v2/webrev/
>
> src/share/vm/ci/ciInstanceKlass.cpp:
>
> + bool ciInstanceKlass::is_boxed_value_offset(int offset) const {
>
> I wanted to ask why don't you check is_loaded() but my question now is:  why don't you call is_box_klass() from is_boxed_value_offset()?

It works without is_loaded() because method is_boxed_value_offset() is 
only called in TypeOopPtr() when klass is exact and loaded. But I will 
add it so the method could be used in other places.

I did not call is_box_klass() to avoid duplicated calls
SystemDictionary::box_klass_type(get_Klass()). I can add an other 
private method is_box_klass(BasicType bt) to be used from these public 
methods.

>
> src/share/vm/opto/bytecodeInfo.cpp:
>
> + static bool is_unboxing(ciMethod* callee_method, Compile* C) {
>
> Can we also name it is_unboxing_method?

Yes, will do.

>
> src/share/vm/opto/callGenerator.cpp:
>
> !   for (uint i1 = 0; i1 < nargs; i1++) {
> +     map->set_req(jvms->argoff() + i1, call->in(TypeFunc::Parms + i1));
>      }
>
> Use map->set_argument instead of map->set_req.  We should change this wherever we touch code.

Right.

>
> src/share/vm/opto/callnode.cpp:
>
> + //---------------------------clone_deep----------------------------------------
> + void JVMState::set_map_deep(SafePointNode* map) {
>
> Name wrong in comment.  Could you use Doxygen format here?

Will do.

>
> src/share/vm/opto/callnode.hpp:
>
>      int            arg_size() const { return monoff() - argoff(); }
>
> Why did you remove this one?

It is not used and it is not correct. For example, in do_late_inline() 
the stack is expanded to max size and it will not match number of 
arguments on stack. In general _sp can point in any offset inside stack 
and you can have less arguments than _monoff - _sp.

>
> +   bool is_autoboxing() const {
> +     return is_macro() && (method() != NULL) && method()->is_boxing_method();
> +   }
>
> We should either use "autoboxing" or "boxing".  Mixing them is confusing.

I will change ciMethod::is_boxing_method()

>
> src/share/vm/opto/compile.cpp:
>
> +                   _eliminate_autobox(eliminate_boxing),
>
> Same here.
I will change name of eliminate_boxing parameter.

>
> src/share/vm/opto/compile.hpp:
>
> +   bool has_late_inline() const { return ((_late_inlines.length() + _string_late_inlines.length()) > 0); }
>
> This method is not used.  If you want to keep it, don't you also have to add _boxing_late_inlines.length()?

I will remove it. I was thinking to to use it to guard call to 
inline_incrementally() which process only _late_inlines and 
_string_late_inlines.

>
> src/share/vm/opto/escape.cpp:
>
> !   for( int i=0; i < cnt; i++ ) {
> !   for (int i=0; i < cnt; i++) {
>
> When you're already changing the parens could also change spacing around "="?

Will do.

>
> src/share/vm/opto/memnode.cpp:
>
> +         switch (constant.basic_type()) {
> +           case T_BOOLEAN:  return TypeInt::make(constant.as_boolean());
> +           case T_INT:      return TypeInt::make(constant.as_int());
> +           case T_CHAR:     return TypeInt::make(constant.as_char());
> +           case T_BYTE:     return TypeInt::make(constant.as_byte());
> +           case T_SHORT:    return TypeInt::make(constant.as_short());
> +           case T_FLOAT:    return TypeF::make(constant.as_float());
> +           case T_DOUBLE:   return TypeD::make(constant.as_double());
> +           case T_LONG:     return TypeLong::make(constant.as_long());
> +           default:
> +             ShouldNotReachHere();
> +         }
>
> This might be useful in a method for later reuse.

I will add Type::get_const_boxed_value(ciConstant constant)

>
> src/share/vm/opto/node.cpp:
>
> + //----------------------------replace_edge_in_range-----------------------------
> + int Node::replace_edges_in_range(Node* old, Node* neww, int start, int end) {
>
> Name wrong in comment.  Doxygen? ;-)

Okay.

>
> Why are there no tests for Character and Short?

They are not different from Integer. Byte is different since it use only 
cache array:

     public static Byte valueOf(byte b) {
         final int offset = 128;
         return ByteCache.cache[(int)b + offset];
     }

thanks,
Vladimir

>
> -- Chris
>
>>
>> Resurrected autobox elimination code and enabled it by default.
>> Added new experimental C2 flag in preparation for 8012974.
>>
>> After static fields were moved to JavaMirror the existing autobox elimination code (LoadNode::eliminate_autobox()) stopped working because graph was changed. Fixed it for loading from Boxing cache array, moved spit through Phi code from eliminate_autobox() to LoadNode::split_through_phi(). Also use incremental inlining to delay inlining of valueOf() methods to get value from argument. Mark such method as Macro nodes and treat them the same way as Allocations. Added value load from constant boxing objects. Eliminated MemBarRelease for non-escaping allocations (such membar generated when final fields are initialized in constructor).
>>
>> Did not get any significant performance improvements because very few places have opportunity for current optimization (when all objects do not escape).
>>
>> Additional changes:
>> Added ability to recompile method without EliminateAutoBox if compilation with it failed.
>>
>> Added assert live_nodes < MaxNodeLimit into Node costructor.
>>
>> Added timer for incremental inlining.
>>
>> Fixed TypeAryPtr::narrow_size_type() to return correct type for arr[MAXINT] (currently it returns incorrect arr[0]).
>>
>> Moved the check in assert to condition in loopPredicate.cpp because I hit the problem during CTW testing.
>>
>> Fixed tiered compilation process dependence on the order of -Xcomp -XX:+TieredCompilation flags.
>>
>> Added regression tests.
>>
>> Tested with jprt, ctw, nashorn.
>>
>> Thanks,
>> Vladimir
>>
>