Re: invert_tree_comparsion tweek for bettter inline predicates

[Richard Biener <rguenther at suse dot de>]

On Tue, 14 Apr 2015, Jan Hubicka wrote:

> Hi,
> while looking on a testcase, i noticed that for simple code
> 
>   if (param > 6.0)
>     BB1;
>   else
>     BB2;
> the inline predicates currectly determine (param > 6.0) predicate
> for BB1, but they give (true) predicate to BB2 unless -fno-trapping-math is
> specified.  This is because invert_tree_comparison returns ERROR_MARK
> when called with HONOR_NANS.
> 
> I see that this is most likely to preserve trappingness of the comparsion
> - UNLE that is a logcial negative won't report unordered.  For my use it is
>   however save to use (param UNLE 6.0) as predicate for BB2 as I do not really
> care if the conditional will trap.  The inline predicates just need to be
> conservative with respect to real program behaviour.
> I can not pass HONOR_NANS=false because then the BB2 predicate would
> be (param <= 6.0) and that would allow inliner to assume that for
> param == UNORDERED both code paths are unreachable.
> 
> This patch adds extra paremter to invert_tree_comparsion to do what I want.
> Does it seem sane?

You could also temporarily un-set flag_trapping_math around the call.

That said, invert_tree_comparison should get flag_trapping_math
as argument, not another that ignores it.  That would be a cleaner
interface.

You can add an inline overload

inline enum tree_code
invert_tree_comparison (enum tree_code code, bool honor_nans)
{
  return invert_tree_comparison (code, honor_nans, flag_trapping_math);
}

to make old code work without change.

Richard.

> Honza
> 
> 	* fold-const.c (invert_tree_comparison): Add CONSIDER_TRAP.
> 	* fold-const.h (invert_tree_comparison): Update prototype.
> 	* ipa-inline-analysis.c (add_clause): Update use
> 	of invert_tree_comparsoin
> 	(set_cond_stmt_execution_predicate): Likewise.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c	(revision 222052)
> +++ fold-const.c	(working copy)
> @@ -2436,13 +2436,17 @@ pedantic_non_lvalue_loc (location_t loc,
>  /* Given a tree comparison code, return the code that is the logical inverse.
>     It is generally not safe to do this for floating-point comparisons, except
>     for EQ_EXPR, NE_EXPR, ORDERED_EXPR and UNORDERED_EXPR, so we return
> -   ERROR_MARK in this case.  */
> +   ERROR_MARK in this case.
> +
> +   if CONSDIER_TRAP is false, do not really care about the case when
> +   conditional expression may trap.  */
>  
>  enum tree_code
> -invert_tree_comparison (enum tree_code code, bool honor_nans)
> +invert_tree_comparison (enum tree_code code, bool honor_nans,
> +		        bool consider_trap)
>  {
> -  if (honor_nans && flag_trapping_math && code != EQ_EXPR && code != NE_EXPR
> -      && code != ORDERED_EXPR && code != UNORDERED_EXPR)
> +  if (honor_nans && consider_trap && flag_trapping_math && code != EQ_EXPR
> +      && code != NE_EXPR && code != ORDERED_EXPR && code != UNORDERED_EXPR)
>      return ERROR_MARK;
>  
>    switch (code)
> Index: fold-const.h
> ===================================================================
> --- fold-const.h	(revision 222052)
> +++ fold-const.h	(working copy)
> @@ -126,7 +126,8 @@ extern bool tree_swap_operands_p (const_
>  extern enum tree_code swap_tree_comparison (enum tree_code);
>  
>  extern bool ptr_difference_const (tree, tree, HOST_WIDE_INT *);
> -extern enum tree_code invert_tree_comparison (enum tree_code, bool);
> +extern enum tree_code invert_tree_comparison (enum tree_code, bool,
> +					      bool consider_trap = false);
>  
>  extern bool tree_unary_nonzero_warnv_p (enum tree_code, tree, tree, bool *);
>  extern bool tree_binary_nonzero_warnv_p (enum tree_code, tree, tree, tree op1,
> Index: ipa-inline-analysis.c
> ===================================================================
> --- ipa-inline-analysis.c	(revision 222052)
> +++ ipa-inline-analysis.c	(working copy)
> @@ -381,7 +381,8 @@ add_clause (conditions conditions, struc
>  		&& cc2->code != IS_NOT_CONSTANT
>  		&& cc2->code != CHANGED
>  		&& cc1->code == invert_tree_comparison (cc2->code,
> -							HONOR_NANS (cc1->val)))
> +							HONOR_NANS (cc1->val),
> +							false))
>  	      return;
>  	  }
>      }
> @@ -1798,15 +1799,13 @@ set_cond_stmt_execution_predicate (struc
>    if (unmodified_parm_or_parm_agg_item (info, last, op, &index, &aggpos))
>      {
>        code = gimple_cond_code (last);
> -      inverted_code = invert_tree_comparison (code, HONOR_NANS (op));
> +      inverted_code = invert_tree_comparison (code, HONOR_NANS (op), false);
>  
>        FOR_EACH_EDGE (e, ei, bb->succs)
>  	{
>  	  enum tree_code this_code = (e->flags & EDGE_TRUE_VALUE
>  				      ? code : inverted_code);
> -	  /* invert_tree_comparison will return ERROR_MARK on FP
> -	     comparsions that are not EQ/NE instead of returning proper
> -	     unordered one.  Be sure it is not confused with NON_CONSTANT.  */
> +	  /*  Be sure to not confuse ERROR_MARK with NON_CONSTANT.  */
>  	  if (this_code != ERROR_MARK)
>  	    {
>  	      struct predicate p = add_condition (summary, index, &aggpos,
> 
> 

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