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

[Roland Westrelin]

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

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.

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.