[PATCH] Improve in_array_bounds_p

[Richard Biener]

On Fri, 10 Jul 2015, Richard Biener wrote:

> On Fri, 10 Jul 2015, Richard Biener wrote:
> 
> > 
> > I was just testing the patch below which runs into latent issues when
> > building libjava (at least)...
> > 
> > /space/rguenther/src/svn/trunk/libjava/java/lang/natClassLoader.cc: In 
> > function Β‘java::lang::Class* _Jv_FindClassInCache(_Jv_Utf8Const*)Β’:
> > /space/rguenther/src/svn/trunk/libjava/java/lang/natClassLoader.cc:97:1: 
> > error:BB 3 last statement has incorrectly set lp
> >  _Jv_FindClassInCache (_Jv_Utf8Const *name)
> >  ^
> > /space/rguenther/src/svn/trunk/libjava/java/lang/natClassLoader.cc:97:1: 
> > internal compiler error: verify_flow_info failed
> > 0x8e2132 verify_flow_info()
> >         /space/rguenther/src/svn/trunk/gcc/cfghooks.c:261
> > 
> > so I have to debug that first.
> 
> It's stmts no longer throwing after VRP setting a value-range on
> an array index for example.  I've addressed this in the revised
> patch below which teaches CFG cleanup to deal with this (it
> already removes dead EH edges and makes similar adjustments for
> noreturn calls).
> 
> >  Still IMHO the patch makes sense apart
> > from the ugly need to go through a INTEGER_CST tree when converting
> > a wide_int to a widest_int (ugh).  Any wide-int folks around that
> > can suggest something better here (reason: the two integers we compare
> > do not have to have the same type/precision - see tree_int_cst_lt
> > which also uses widest_ints).
> 
> This issue still remains.

Fixed with widest_int::from (idx_min, TYPE_SIGN (TREE_TYPE (idx))).

But with the patch we run into the general issue that changing
a context insensitive predicate to use context sensitive information
leads to wrong-code.  (Only) gcc.c-torture/execute/pr51933.c
fails because if-conversion sees that v2[_18] cannot trap in

  if (u_14 <= 255)
    goto <bb 8>;
  else
    goto <bb 9>;

  <bb 8>:
  # RANGE [0, 255] NONZERO 255
  _18 = (int) u_14;
  _19 = v2[_18];

but of course it uses it to move v2[_18] out of its controling
condition.  As the patch was supposed to improve if-conversion
in the first place (and other passes might be similar confused)
I retract it.

We'd need a more specialized predicate that also gets
context information.

Richard.

> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
> 
> Richard.
> 
> 2015-07-10  Richard Biener  <rguenther@suse.de>
> 
> 	* tree-eh.c (in_array_bounds_p): Use value-range information
> 	when available.
> 	* tree-cfgcleanup.c (cleanup_control_flow_bb): Clean stmts
> 	from stale EH info.
> 
> Index: gcc/tree-eh.c
> ===================================================================
> --- gcc/tree-eh.c	(revision 225655)
> +++ gcc/tree-eh.c	(working copy)
> @@ -2532,8 +2532,11 @@ in_array_bounds_p (tree ref)
>  {
>    tree idx = TREE_OPERAND (ref, 1);
>    tree min, max;
> +  wide_int idx_min, idx_max;
>  
> -  if (TREE_CODE (idx) != INTEGER_CST)
> +  if (TREE_CODE (idx) != INTEGER_CST
> +      && (TREE_CODE (idx) != SSA_NAME
> +	  || get_range_info (idx, &idx_min, &idx_max) != VR_RANGE))
>      return false;
>  
>    min = array_ref_low_bound (ref);
> @@ -2544,11 +2547,26 @@ in_array_bounds_p (tree ref)
>        || TREE_CODE (max) != INTEGER_CST)
>      return false;
>  
> -  if (tree_int_cst_lt (idx, min)
> -      || tree_int_cst_lt (max, idx))
> -    return false;
> +  if (TREE_CODE (idx) == INTEGER_CST)
> +    {
> +      if (tree_int_cst_lt (idx, min)
> +	  || tree_int_cst_lt (max, idx))
> +	return false;
> +
> +      return true;
> +    }
> +  else
> +    {
> +      if (wi::lts_p (wi::to_widest (wide_int_to_tree (TREE_TYPE (idx),
> +						      idx_min)),
> +		     wi::to_widest (min))
> +	  || wi::lts_p (wi::to_widest (max),
> +			wi::to_widest (wide_int_to_tree (TREE_TYPE (idx),
> +							 idx_max))))
> +	return false;
>  
> -  return true;
> +      return true;
> +    }
>  }
>  
>  /* Returns true if it is possible to prove that the range of
> Index: gcc/tree-cfgcleanup.c
> ===================================================================
> --- gcc/tree-cfgcleanup.c	(revision 225662)
> +++ gcc/tree-cfgcleanup.c	(working copy)
> @@ -256,6 +256,14 @@ cleanup_control_flow_bb (basic_block bb)
>             && remove_fallthru_edge (bb->succs))
>      retval = true;
>  
> +  /* If a stmt may no longer throw, remove it from the EH tables
> +     and cleanup dead EH edges.  */
> +  else if (maybe_clean_eh_stmt (stmt))
> +    {
> +      gimple_purge_dead_eh_edges (bb);
> +      retval = true;
> +    }
> +
>    return retval;
>  }
>  

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)