RFR (L): 7153771: array bound check elimination for c1

[Christian Thalinger]

On Mar 19, 2013, at 2:19 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Hi Chris, 
> 
> Thanks for reviewing this.
> 
> Here is a new webrev:
> http://cr.openjdk.java.net/~roland/7153771/webrev.04/
> 
>> src/share/vm/c1/c1_Compilation.hpp:
>> 
>> +  bool is_optimistic() const                             {
>> +    return !TieredCompilation && 
>> 
>> Do you know why optimistic RCE doesn't work with tiered?
> 
> It's not that it doesn't work. Rather it probably doesn't make much sense. We probably want to get as quickly as possible to a c2 compiled method and having a recompilation with c1 caused by an invalid predicate would delay the c2 code without much benefit.

That makes sense.

> 
>> src/share/vm/c1/c1_Instruction.cpp:
>> 
>> +    case aeq: assert(false, "Above equal cannot be negated");
>> +    case beq: assert(false, "Below equal cannot be negated");
>> 
>> They can be negated but I guess we don't need it?
> 
> They can't be negated with available condition values.
> 
>> src/share/vm/c1/c1_InstructionPrinter.cpp:
>> 
>> +    case If::aeq: return "aeq";
>> +    case If::beq: return "beq"; 
>> 
>> Graal uses this notation for unsigned compares:
>> 
>>   AE("|>=|"),
>>   BE("|<=|"),
>> 
>> Can we use these too for the sake of limited human parsing abilities?
> 
> Ok.
> 
>> src/share/vm/c1/c1_IR.cpp:
>> 
>> +  virtual void block_do(BlockBegin *block) {
>> +
>> +    Instruction *cur = block;
>> +    while (cur) {
>> +      assert(cur->block() == block, "Block begin is not correct");
>> +      cur = cur->next();
>> +    }
>> +  }
>> 
>> Just a nit but a for loop would be nicer here.
> 
> Ok, I'll make the change.
> 
>> 
>> src/share/vm/oops/methodData.cpp:
>> 
>> int MethodData::bytecode_cell_count(Bytecodes::Code code) {
>> +#if defined(COMPILER1) && !defined(COMPILER2)
>> +  return no_profile_data;
>> +#endif
>>  switch (code) {
>> 
>> Aren't some compilers warning about unreached code?
> 
> Are you suggesting the rest of the method be enclosed in a #else?

Yes.

> 
>> src/share/vm/c1/c1_RangeCheckElimination.cpp:
>> 
>> Could we factor some code in RangeCheckEliminator::insert_deoptimization?  E.g.
>> 
>> 697       Constant *constant = new Constant(new IntConstant(upper));
>> 698       NOT_PRODUCT(constant->set_printable_bci(ai->printable_bci()));
>> 699       insert_position = insert_position->insert_after(constant);
>> 700       ArithmeticOp *ao = new ArithmeticOp(Bytecodes::_iadd, constant, upper_instr, false, NULL);
>> 701       insert_position = insert_position->insert_after_same_bci(ao);
>> 702       // Compare for geq array.length
>> 703       RangeCheckPredicate *deoptimize = new RangeCheckPredicate(ao, Instruction::geq, true, length_instr, state->copy());
>> 704       NOT_PRODUCT(deoptimize->set_printable_bci(ai->printable_bci()));
>> 705       insert_position = insert_position->insert_after_same_bci(deoptimize);
>> 
>> And perhaps some other code.
> 
> Ok. I've reworked insert_deoptimization.

Much better!  Thank you.

> 
>> 
>> RangeCheckEliminator::process_access_indexed:
>> 
>> 824         TRACE_RANGE_CHECK_ELIMINATION(
>> 825           tty->fill_to(block->dominator_depth()*2);
>> 826         tty->print_cr("Lower instruction %d not loop invariant!", lower_instr->id())); 
>> 
>> The indenting is wrong here and very hard to read.  Can we change all occurrences to:
>> 
>> 824         TRACE_RANGE_CHECK_ELIMINATION(
>> 825           tty->fill_to(block->dominator_depth()*2);
>> 826           tty->print_cr("Lower instruction %d not loop invariant!", lower_instr->id());
>>            );
> 
> Ok.

Thank you.  Looks good.

-- Chris

> 
> Roland.
>