Fix reassoc ICE (PR tree-optimization/69802)

Hi!

The following patch fixes an ICE where one of the range tests
is SSA_NAME_DEF_STMT of a bool/_Bool or unsigned : 1 bitfield.
In that case, we don't know where to put the adjusted range test.
The patch for this uncommon case gives up, unless the range test
can be the SSA_NAME_DEF_STMT itself, and in that case makes sure we
DTRT.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-15  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/69802
	* tree-ssa-reassoc.c (update_range_test): If op is
	SSA_NAME_IS_DEFAULT_DEF, give up unless tem is a positive
	op == 1 test of precision 1 integral op, otherwise handle
	that case as op itself.  Fix up formatting.
	(optimize_range_tests_to_bit_test, optimize_range_tests): Fix
	up formatting.

	* gcc.dg/pr69802.c: New test.


	Jakub

Hi,

On Mon, 15 Feb 2016, Jakub Jelinek wrote:

> +  /* If op is default def SSA_NAME, there is no place to insert the
> +     new comparison.  Give up, unless we can use OP itself as the
> +     range test.  */
> +  if (op && SSA_NAME_IS_DEFAULT_DEF (op))
> +    {
> +      if (op == range->exp
> +	  && ((TYPE_PRECISION (optype) == 1 && TYPE_UNSIGNED (optype))
> +	      || TREE_CODE (optype) == BOOLEAN_TYPE)
> +	  && (op == tem
> +	      || (TREE_CODE (tem) == EQ_EXPR
> +		  && TREE_OPERAND (tem, 0) == op
> +		  && integer_onep (TREE_OPERAND (tem, 1))))
> +	  && opcode != BIT_IOR_EXPR
> +	  && (opcode != ERROR_MARK || oe->rank != BIT_IOR_EXPR))

Perhaps just give up always, instead of this complicated (and hence 
fragile) hackery?  Are you 100% sure you catched everything, are there 
testcases for each part of the condition (I miss at least one proving 
that making the condition true is correct)?


Ciao,
Michael.

On Mon, Feb 15, 2016 at 10:27:16PM +0100, Michael Matz wrote:
> On Mon, 15 Feb 2016, Jakub Jelinek wrote:
> 
> > +  /* If op is default def SSA_NAME, there is no place to insert the
> > +     new comparison.  Give up, unless we can use OP itself as the
> > +     range test.  */
> > +  if (op && SSA_NAME_IS_DEFAULT_DEF (op))
> > +    {
> > +      if (op == range->exp
> > +	  && ((TYPE_PRECISION (optype) == 1 && TYPE_UNSIGNED (optype))
> > +	      || TREE_CODE (optype) == BOOLEAN_TYPE)
> > +	  && (op == tem
> > +	      || (TREE_CODE (tem) == EQ_EXPR
> > +		  && TREE_OPERAND (tem, 0) == op
> > +		  && integer_onep (TREE_OPERAND (tem, 1))))
> > +	  && opcode != BIT_IOR_EXPR
> > +	  && (opcode != ERROR_MARK || oe->rank != BIT_IOR_EXPR))
> 
> Perhaps just give up always, instead of this complicated (and hence 
> fragile) hackery?  Are you 100% sure you catched everything, are there 

It is IMO not that fragile.  The
op == range->exp is quite obvious condition, it could have been even assert
instead.
The second condition is what is used e.g. in init_range_test, i.e.
bool/unsigned :1 only.  The third - op == tem is obviously good
transformation to tem = op, so the patch just makes sure we don't ICE
say in gsi_for_stmt (stmt); that is what we get for bool and is covered
in the testcase.  The EQ_EXPR case is what we get for unsigned : 1 instead.
Perhaps it could be also op != 0 instead of just op == 1.
And lastly, the BIT_IOR_EXPR tests are to make sure we don't
  if (opcode == BIT_IOR_EXPR
      || (opcode == ERROR_MARK && oe->rank == BIT_IOR_EXPR))
    tem = invert_truthvalue_loc (loc, tem);
a few lines later.  The reason to perform the check earlier is just
to avoid printing it in the dumpfile that we are changing something if we
give up instead.

	Jakub

On Mon, 15 Feb 2016, Jakub Jelinek wrote:

> Hi!
> 
> The following patch fixes an ICE where one of the range tests
> is SSA_NAME_DEF_STMT of a bool/_Bool or unsigned : 1 bitfield.
> In that case, we don't know where to put the adjusted range test.
> The patch for this uncommon case gives up, unless the range test
> can be the SSA_NAME_DEF_STMT itself, and in that case makes sure we
> DTRT.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Richard.

> 2016-02-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/69802
> 	* tree-ssa-reassoc.c (update_range_test): If op is
> 	SSA_NAME_IS_DEFAULT_DEF, give up unless tem is a positive
> 	op == 1 test of precision 1 integral op, otherwise handle
> 	that case as op itself.  Fix up formatting.
> 	(optimize_range_tests_to_bit_test, optimize_range_tests): Fix
> 	up formatting.
> 
> 	* gcc.dg/pr69802.c: New test.
> 
> --- gcc/tree-ssa-reassoc.c.jj	2016-02-12 10:23:51.000000000 +0100
> +++ gcc/tree-ssa-reassoc.c	2016-02-15 14:25:54.996572238 +0100
> @@ -2046,19 +2046,41 @@ update_range_test (struct range_entry *r
>  {
>    operand_entry *oe = (*ops)[range->idx];
>    tree op = oe->op;
> -  gimple *stmt = op ? SSA_NAME_DEF_STMT (op) :
> -    last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id));
> +  gimple *stmt = op ? SSA_NAME_DEF_STMT (op)
> +		    : last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id));
>    location_t loc = gimple_location (stmt);
>    tree optype = op ? TREE_TYPE (op) : boolean_type_node;
>    tree tem = build_range_check (loc, optype, unshare_expr (exp),
>  				in_p, low, high);
>    enum warn_strict_overflow_code wc = WARN_STRICT_OVERFLOW_COMPARISON;
>    gimple_stmt_iterator gsi;
> -  unsigned int i;
> +  unsigned int i, uid;
>  
>    if (tem == NULL_TREE)
>      return false;
>  
> +  /* If op is default def SSA_NAME, there is no place to insert the
> +     new comparison.  Give up, unless we can use OP itself as the
> +     range test.  */
> +  if (op && SSA_NAME_IS_DEFAULT_DEF (op))
> +    {
> +      if (op == range->exp
> +	  && ((TYPE_PRECISION (optype) == 1 && TYPE_UNSIGNED (optype))
> +	      || TREE_CODE (optype) == BOOLEAN_TYPE)
> +	  && (op == tem
> +	      || (TREE_CODE (tem) == EQ_EXPR
> +		  && TREE_OPERAND (tem, 0) == op
> +		  && integer_onep (TREE_OPERAND (tem, 1))))
> +	  && opcode != BIT_IOR_EXPR
> +	  && (opcode != ERROR_MARK || oe->rank != BIT_IOR_EXPR))
> +	{
> +	  stmt = NULL;
> +	  tem = op;
> +	}
> +      else
> +	return false;
> +    }
> +
>    if (strict_overflow_p && issue_strict_overflow_warning (wc))
>      warning_at (loc, OPT_Wstrict_overflow,
>  		"assuming signed overflow does not occur "
> @@ -2096,12 +2118,22 @@ update_range_test (struct range_entry *r
>      tem = invert_truthvalue_loc (loc, tem);
>  
>    tem = fold_convert_loc (loc, optype, tem);
> -  gsi = gsi_for_stmt (stmt);
> -  unsigned int uid = gimple_uid (stmt);
> +  if (stmt)
> +    {
> +      gsi = gsi_for_stmt (stmt);
> +      uid = gimple_uid (stmt);
> +    }
> +  else
> +    {
> +      gsi = gsi_none ();
> +      uid = 0;
> +    }
> +  if (stmt == NULL)
> +    gcc_checking_assert (tem == op);
>    /* In rare cases range->exp can be equal to lhs of stmt.
>       In that case we have to insert after the stmt rather then before
>       it.  If stmt is a PHI, insert it at the start of the basic block.  */
> -  if (op != range->exp)
> +  else if (op != range->exp)
>      {
>        gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
>        tem = force_gimple_operand_gsi (&gsi, tem, true, NULL_TREE, true,
> @@ -2489,7 +2521,7 @@ optimize_range_tests_to_bit_test (enum t
>  	  operand_entry *oe = (*ops)[ranges[i].idx];
>  	  tree op = oe->op;
>  	  gimple *stmt = op ? SSA_NAME_DEF_STMT (op)
> -			   : last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id));
> +			    : last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id));
>  	  location_t loc = gimple_location (stmt);
>  	  tree optype = op ? TREE_TYPE (op) : boolean_type_node;
>  
> @@ -2553,7 +2585,7 @@ optimize_range_tests_to_bit_test (enum t
>  	  gcc_assert (TREE_CODE (exp) == SSA_NAME);
>  	  gimple_set_visited (SSA_NAME_DEF_STMT (exp), true);
>  	  gimple *g = gimple_build_assign (make_ssa_name (optype),
> -					  BIT_IOR_EXPR, tem, exp);
> +					   BIT_IOR_EXPR, tem, exp);
>  	  gimple_set_location (g, loc);
>  	  gimple_seq_add_stmt_without_update (&seq, g);
>  	  exp = gimple_assign_lhs (g);
> @@ -2599,8 +2631,9 @@ optimize_range_tests (enum tree_code opc
>        oe = (*ops)[i];
>        ranges[i].idx = i;
>        init_range_entry (ranges + i, oe->op,
> -			oe->op ? NULL :
> -			  last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id)));
> +			oe->op
> +			? NULL
> +			: last_stmt (BASIC_BLOCK_FOR_FN (cfun, oe->id)));
>        /* For | invert it now, we will invert it again before emitting
>  	 the optimized expression.  */
>        if (opcode == BIT_IOR_EXPR
> --- gcc/testsuite/gcc.dg/pr69802.c.jj	2016-02-15 14:06:22.716491146 +0100
> +++ gcc/testsuite/gcc.dg/pr69802.c	2016-02-15 14:05:58.000000000 +0100
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/69802 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wall" } */
> +
> +struct S { unsigned f : 1; };
> +int a, d;
> +
> +int
> +foo (void)
> +{
> +  unsigned b = 0;
> +  struct S c;
> +  d = ((1 && b) < c.f) & c.f;	/* { dg-warning "is used uninitialized" } */
> +  return a;
> +}
> +
> +int
> +bar (_Bool c)
> +{
> +  unsigned b = 0;
> +  d = ((1 && b) < c) & c;
> +  return a;
> +}
> 
> 	Jakub
> 
>