RFR (L): 7173584: Implement arraycopy as a macro node

Roland Westrelin roland.westrelin at oracle.com
Sat Aug 2 04:46:14 UTC 2014


Here is a new webrev with the gvn_transform_ctrl -> record_for_igvn renaming:

http://cr.openjdk.java.net/~roland/7173584/webrev.04/

Roland.

Roland Westrelin <roland.westrelin at oracle.com> writes:

> Thanks for the review, Vladimir. See comments inlined:
>
>>> Here is a new webrev for this after a long time:
>>> 
>>> http://cr.openjdk.java.net/~roland/7173584/webrev.03/
>> 
>> Looks reasonable.
>> 
>>> 
>>> This addresses the comments below (replies inlined):
>>> 
>>>> I don't think you need suffix "_any_phase" in the name gen_subtype_check_any_phase(). Just add comment that it is also used in phase Macro. Generally speaking GraphKit class is wrong place for it. May be it should be in base class Phase but keep the code in graphKit.cpp file.
>>>> 
>>>> You don't need special gvn_transform(). It is already done since Macro phase sets gvn._delay_transform flag to true and:
>>>> 
>>>> Node *PhaseIterGVN::transform( Node *n ) {
>>>>  if (_delay_transform) {
>>>>    // Register the node but don't optimize for now
>>>>    register_new_node_with_optimizer(n);
>>>>    return n;
>>>>  }
>>>> 
>>>> gvn_transform_ctrl() should remain to be record_for_igvn() but virtual as you suggested.
>>> 
>>> BTW, is it required that transform_ctrl() does nothing for PhaseIterGVN? Or is it simply that it is a waste of resources to process a Region node, for instance, before its inputs?
>> 
>> +   gvn->transform(iff);
>> +   if (!bol->is_Con()) gvn->transform_ctrl(iff);
>> 
>> During Parse record_for_igvn() puts nodes on worklist which will populate initial igvn._worklist. register_new_node_with_optimizer(n) does that already when IGVN transformation is delayed. So PhaseIterGVN::transform() will do what record_for_igvn() does.
>> 
>> transform_ctrl() is not good name. record_for_igvn() is very good. Why not use it?
>> 
>> phaseX.hpp
>> 
>> +   virtual void record_for_igvn(Node *n) {
>> +     C->record_for_igvn(n);
>> +   }
>
> Ok. And:
>
> virtual void record_for_igvn(Node *n) { }
>
> for PhaseIterGVN?
>
>>>> It would be also nice to factor out IfNode creation into separate method (it used 5 times):
>>>> 
>>>> +        Node* cmp = gvn_transform(new(C, 3) CmpPNode(subklass, superklass), gvn);
>>>> +        Node* bol = gvn_transform(new(C, 2) BoolNode(cmp, BoolTest::eq), gvn);
>>>> +        IfNode* iff = new (C, 2) IfNode(*ctrl, bol, PROB_STATIC_FREQUENT, COUNT_UNKNOWN)
>>>> +        gvn_transform(iff, gvn);
>>>> +        if (!bol->is_Con()) gvn_transform_ctrl(iff, gvn);
>>>> 
>>>> 
>>>> Next optimization belongs to ConvI2LNode::Value(). May be we should not widen type in ConvI2LNode::Ideal() and Value() if input is singleton (constant).
>>>> 
>>>> +  Node *chk_off_X = LP64_ONLY(NULL) NOT_LP64(chk_off);
>>>> +#ifdef _LP64
>>>> +  jint chk_off_con = gvn->find_int_con(chk_off, Type::OffsetBot);
>>>> +  if (chk_off_con != Type::OffsetBot) {
>>>> +    chk_off_X = gvn->longcon((jlong) chk_off_con);
>>>> +  } else {
>>>> +    chk_off_X = gvn_transform(new (C, 2) ConvI2LNode(chk_off), gvn);
>>>> +  }
>>>> +#endif
>>> 
>>> I removed this and didn’t make any change to ConvI2LNode::Value() and ConvI2LNode::Ideal().
>>> The widening uses MAX2((jlong)in_type->_lo, lo1), MIN2((jlong)in_type->_hi, hi1) as new bounds. If the input is constant and falls within [lo1, hi1] then the result is [(jlong)in_type->_lo, (jlong)in_type->_hi] which is constant. Only if the input is constant and doesn’t fall within [lo1, hi1] would there be a problem. But this inconsistency between the ConvI2L and its input is unexpected, right? It doesn’t seem to ever happen in practice.
>> 
>> It happens. ConvI2L node is usually generated after range check of array index and its type is set to [0,arr.length]. Nothing prevents to have negative constant as index. The array access will not be executed then but ConvI2L's and input's types may not overlap.
>
> But then the load is control dependent on the range check, the ConvI2L is input to the load. If the index is a constant that doesn’t fit in the range, only the branch of the range check that fails is kept, the load is eliminated and so is the ConvI2L?
>
> FWIW, I ran a bunch of tests with this debug code:
>
> diff --git a/src/share/vm/opto/phaseX.cpp b/src/share/vm/opto/phaseX.cpp
> --- a/src/share/vm/opto/phaseX.cpp
> +++ b/src/share/vm/opto/phaseX.cpp
> @@ -1049,7 +1049,12 @@
>      set_type_bottom(n);
>    }
>
> -  return transform_old(n);
> +  bool is_constant = n->Opcode() == Op_ConvI2L && n->in(1) && type(n->in(1))->is_int()->is_con();
> +  Node* res = transform_old(n);
> +  if (is_constant && !type(res)->is_long()->is_con()) {
> +    fatal("XXXXXXXXXXXX");
> +  }
> +  return res;
>  }
>
>  Node *PhaseIterGVN::transform_old(Node* n) {
>
> and it never failed.
>
> Roland.
>
>> 
>> Vladimir
>> 
>>> 
>>> ConvI2LNode::Value() computes the type with tl = tl->filter(_type) so if the input is constant, the result is constant as well unless tl and _type are disjoint. Why would that happen?
>>> 
>>> Roland.
>>> 
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> 
>>>> Roland Westrelin wrote:
>>>>>> Also instead of duplicating gen_subtype_check() in macroArrayCopy.cpp can you modify the original one to work for you (by passing additional flag)?
>>>>> This (on top of previous webrev - I've done only minimal testing)?
>>>>> http://cr.openjdk.java.net/~roland/7173584/webrev.02/
>>>>> Or do we want gvn_transform/gvn_transform_ctrl to be virtual methods of PhaseGVN/PhaseIterGVN?
>>>>> Roland.


More information about the hotspot-compiler-dev mailing list