My Patch for Defect 6478991

[Tom Rodriguez]

As Christian says your fix isn't really right.  The main thing this code 
it try to do is fold an explicit null check into an implicit one so we 
don't end up with extra checks and state.  Call sites commonly have 
explicit null checks once they are inlined and field accesses have 
implicit null checks so by folding the state of the explicit check into 
the field access we can eliminate extra work.  We have to careful that 
we don't end up reordering exceptions which is what Christians test 
showed.  Any instruction which might throw an exception should clear the 
last explicit null check so that its state doesn't get moved past the 
other exception point.  So we should be calling 
clear_last_explicit_null_check() for some instruction which we're not. 
Does that make it clearer?

tom


rgougol at email.sjsu.edu wrote:
> Hello everybody and thanks for the feedbacks so far,
> 
> Here comes my suggested patch for the defect of NullCheck Elimination. Basically
> it sets a flag if a type check operation is iterated. This flag prevents the 
> optimizations of folding the NullChecks and let the elimination of the latter
> NullCheck exception instead of the former. The flag is unset when the last
> explicit exception is set. I will be very thankful if I get any feedback? Shall
> I extend the patch to all the trap instruction besides type checks? Can also
> somebody explain me what is the purpose of folding null check exception and why
> should the former null check be eliminated instead of the latter? 
> 
> --- openjdk/hotspot/src/share/vm/c1/c1_Optimizer.cpp	2007-10-12
> 00:46:03.000000000 -0700
> +++ nullcheck-openjdk/hotspot/src/share/vm/c1/c1_Optimizer.cpp	2007-12-19
> 14:22:58.000000000 -0800
> @@ -490,6 +490,9 @@
>    // Returns true if caused a change in the block's state.
>    bool      merge_state_for(BlockBegin* block,
>                              ValueSet*   incoming_state);
> +  Instruction *  _prior_type_check;
> +  Instruction *  prior_type_check() const { return _prior_type_check; }
> +  void set_prior_type_check(Instruction * instr) { _prior_type_check = instr; } 
>  
>   public:
>    // constructor
> @@ -498,7 +501,8 @@
>      , _set(new ValueSet())
>      , _last_explicit_null_check(NULL)
>      , _block_states(BlockBegin::number_of_blocks(), NULL)
> -    , _work_list(new BlockList()) {
> +    , _work_list(new BlockList())
> +    , _prior_type_check (NULL)  {
>      _visitable_instructions = new ValueSet();
>      _visitor.set_eliminator(this);
>    }
> @@ -715,6 +719,9 @@
>      // visiting instructions which are references in other blocks or
>      // visiting instructions more than once.
>      mark_visitable(instr);
> +    if (instr->as_TypeCheck() != NULL ) {
> +	set_prior_type_check(instr);
> +    }
>      if (instr->is_root() || instr->can_trap() || (instr->as_NullCheck() != NULL)) {
>        mark_visited(instr);
>        instr->input_values_do(&NullCheckEliminator::do_value);
> @@ -769,13 +776,14 @@
>    Value obj = x->obj();
>    if (set_contains(obj)) {
>      // Value is non-null => update AccessField
> -    if (last_explicit_null_check_obj() == obj && !x->needs_patching()) {
> +    if (last_explicit_null_check_obj() == obj && !x->needs_patching() && !
> prior_type_check()) {
>      } else {
>        x->set_explicit_null_check(NULL);
>        x->set_needs_null_check(false);
> @@ -800,7 +808,7 @@
>    Value array = x->array();
>    if (set_contains(array)) {
>      // Value is non-null => update AccessArray
> -    if (last_explicit_null_check_obj() == array) {
> +    if (last_explicit_null_check_obj() == array && ! prior_type_check()) {
>        x->set_explicit_null_check(consume_last_explicit_null_check());
>        x->set_needs_null_check(true);
>        if (PrintNullCheckElimination) {
> @@ -831,7 +839,7 @@
>    Value array = x->array();
>    if (set_contains(array)) {
>      // Value is non-null => update AccessArray
> -    if (last_explicit_null_check_obj() == array) {
> +    if (last_explicit_null_check_obj() == array && ! prior_type_check()) {
>        x->set_explicit_null_check(consume_last_explicit_null_check());
>        x->set_needs_null_check(true);
>        if (PrintNullCheckElimination) {
> @@ -898,6 +906,7 @@
>      if (PrintNullCheckElimination) {
>        tty->print_cr("NullCheck %d of value %d proves value to be non-null",
> x->id(), obj->id());
>      }
> +    set_prior_type_check(NULL);
>    }
>  }
>  
> 
> Sincerely,
> 
> Rouhollah Gougol
> 
> 
>