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

Mon May 6 18:26:53 PDT 2013

http://cr.openjdk.java.net/~kvn/6934604.v2/webrev.01

Did several renaming 'autobox' -> 'boxing' and other suggested renaming.
Used Doxygen format for new comments.
Added new helper methods:
BasicType ciInstanceKlass::box_klass_type()
const Type* TypeInstPtr::get_const_boxed_value()

thanks,
Vladimir

On 5/6/13 4:31 PM, Christian Thalinger wrote:
>
> On May 6, 2013, at 3:54 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> 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()).
>
> Shouldn't the compiler optimize that?
>
>> 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.
>
> Ok.
>
>>
>>>
>>> +   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];
>
> Got it.
>
> -- Chris
>
>>     }
>>
>> 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
>>>>
>>>
>