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

[Christian Thalinger]

On Feb 20, 2013, at 8:51 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Here is a new version of the webrev that addresses Tom's comments.
> 
> http://cr.openjdk.java.net/~roland/7153771/webrev.01/

src/share/vm/c1/c1_Compilation.hpp:

+  bool is_optimistic() const                             {
+    return !TieredCompilation && 

Do you know why optimistic RCE doesn't work with tiered?

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?

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?

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.

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?

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.

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

It's difficult to verify that the RCE in fact works.  We have to rely on testing.

-- Chris

> 
>> Should this reuse C2 flags like RangeCheckElimination instead of introducing it's own?
> 
> I only reused RangeCheckElimination. I wasn't sure which others it made sense to re-use.
> 
> Roland.
>