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

[Roland Westrelin]

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.

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

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

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

Roland.