RFR(M): 8054478 C2: Incorrectly compiled char[] array access crashes JVM

[Vladimir Kozlov]

On 11/6/14 7:39 AM, Roland Westrelin wrote:
>
>> I need to see new version of webrev. We talked about moving type change from Ideal() to Value(). You are right if the code, currently in ConstraintCastNode::Value(), will be executed first.
>
> I’ll send an updated webrev.
>
>>>>>> opaquenode.cpp
>>>>>>
>>>>>> What test you are talking about: "The pre loop is guarded by a test on an opaque node which is later removed"? I did not get first part of the code. You are putting on worklist a Phi from *previous* (pre-loop) loop. I would understand if you do that for the following (guarded main-, post-) loop, and that is already taking care by putting CastII on worklist.
>>>>>
>>>>> Once the range of values for the pre loop is know, we can optimize the test that guards the main loop. That range of values is only known once the opaque node for the pre loop is removed.
>>>>
>>>> That is what I am asking: "range of values for the pre loop is know" - when this happens, which Opaque1 is removed to make "range" to be known? If it is Opaque1 node from loop_limit_check predicate then we may need to make sure that iteration Phi of pre-loop is put on worklist when predicate's Opaque1 node is removed by cleanup_loop_predicates(). Then you don't need first part in Opaque1Node::Ideal.
>>>
>>> The Opaque1 nodes are the ones created by PhaseIdealLoop::insert_pre_post_loops() (loop limit checks). They are not in the Compile::_predicate_opaqs list and so they are not removed by cleanup_loop_predicates().
>>
>> So how these Opaque1 nodes affects type range of Phi node in pre-loop? That is what I don't understand.
>
> (I know you don’t like when I remove the text from previous emails but that was really getting confusing)

At least leave webrev link so I don't need to search for it in previous 
mails.

>
> PhiNode::Value() has this code:
>
>    // Check for trip-counted loop.  If so, be smarter.
>    CountedLoopNode *l = r->is_CountedLoop() ? r->as_CountedLoop() : NULL;
>    if( l && l->can_be_counted_loop(phase) &&
>        ((const Node*)l->phi() == this) ) { // Trip counted loop!
>      // protect against init_trip() or limit() returning NULL
>      const Node *init   = l->init_trip();
>      const Node *limit  = l->limit();
>      if( init != NULL && limit != NULL && l->stride_is_con() ) {
>        const TypeInt *lo = init ->bottom_type()->isa_int();
>        const TypeInt *hi = limit->bottom_type()->isa_int();
>        if( lo && hi ) {            // Dying loops might have TOP here
>          int stride = l->stride_con();
>          if( stride < 0 ) {          // Down-counter loop
>            const TypeInt *tmp = lo; lo = hi; hi = tmp;
>            stride = -stride;
>          }
>          if( lo->_hi < hi->_lo )     // Reversed endpoints are well defined :-(
>            return TypeInt::make(lo->_lo,hi->_hi,3);
>        }
>      }
>    }
>
> That code can only return something for the induction variable Phi once the opaque node for the loop limit check is removed. That’s why when the opaque node is removed I enqueue the induction variable Phi.

My mistake was that I thought you are looking on Opaque1 node generated 
for main-loop guard:

   // Step B2: Build a zero-trip guard for the main-loop.  After leaving the
   // pre-loop, the main-loop may not execute at all.  Later in life this
   // zero-trip guard will become the minimum-trip guard when we unroll
   // the main-loop.
   Node *min_opaq = new Opaque1Node(C, limit);
   Node *min_cmp  = new CmpINode( pre_incr, min_opaq );


But you are talking about pre-loop exit check:

   // Step B4: Shorten the pre-loop to run only 1 iteration (for now).
   // RCE and alignment may change this later.
   Node *cmp_end = pre_end->cmp_node();
   assert( cmp_end->in(2) == limit, "" );
   Node *pre_limit = new AddINode( init, stride );

   // Save the original loop limit in this Opaque1 node for
   // use by range check elimination.
   Node *pre_opaq  = new Opaque1Node(C, pre_limit, limit);


But in this case you can add check (cl->limit() == this) to be clear 
what you are looking for.
Also instead of looking up through AddI you can look down for 
CountedLoopEnd and get cle->phi() and cle->limit() from it.

Thanks,
Vladimir

>
> In the case of this test case, the pre loop iterates from 0 as long as i > -1. So the Phi for the pre loop has values within [-1, 0]. The main loop is guarded by a test that checks whether The value from the Phi - 1 is greater than 0. With a correct range of values for the Phi from the code above, that checks is statically false and the main loop is optimized away. Otherwise it’s not.
>
> Roland.
>