This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [tree-ssa/mainline PATCH] fold "test"==0 into constant


On Thu, 6 Nov 2003, Jan Hubicka wrote:
> > Isn't it what TREE_OVERFLOW flag is used for?
>
> Hmm, I see it is not.
> I am testing it now, OK if it passes?

Not quite...


> + /* Return true when T is an address and is known to be nonzero.
> +    For floating point we further ensure that T is not denormal.
> +    Similar logic is present in nonzero_address in rtlanal.h  */
> + static bool
> + tree_expr_nonzero_p (tree t)

Blank line between comment and function.

> +     {
> +     case ABS_EXPR:
> +       return ((FLOAT_TYPE_P (type) || (! TREE_UNSIGNED (type) && !flag_wrapv))
> + 	      && tree_expr_nonzero_p (t));
> +

Consistent spacing after "!", atleast within a single expression.

> +     case REAL_CST:
> +       /* ??? We should check !real_zerop (t) && not-denormal.  */
> +       return false;

I think its reasonable to return ! real_zerop here.  See my final
remark below about the issues with denormals.


> +
> +     case PLUS_EXPR:
> +       if (FLOAT_TYPE_P (type) || (! TREE_UNSIGNED (type) && !flag_wrapv))
> + 	{
> + 	  if (tree_expr_nonzero_p (TREE_OPERAND (t, 0)))
> + 	    return tree_expr_nonnegative_p (TREE_OPERAND (t, 1));
> + 	  else if (tree_expr_nonzero_p (TREE_OPERAND (t, 1)))
> + 	    return tree_expr_nonnegative_p (TREE_OPERAND (t, 0));
> + 	  else return false;
> + 	  /* We know that at least one operand is nonzero.
> + 	     If both are positive, we win.  */
> + 	  if (tree_expr_nonnegative_p (TREE_OPERAND (t, 0)
> + 				       && TREE_OPERAND (1, 1)))
> + 	    return true;

Firstly this last call to tree_expr_nonnegative_p is unreachable, as
the if-elseif-else construct above always return.  This is a "good"
because the expression itself makes no sense, and TREE_OPERAND (1, 1)
is likely to do very nasty things :>

Also remember the tree_expr_nonnegative_p does not imply that the
expression is positive.  Some of your logic may be easier to follow
if you introduced a tree_expr_positive_p with an initial implementation
of just "tree_expr_nonnegative_p (op) && tree_expr_nonzerop (op)".

I also think the logic above is clearly wrong.  nonzero(op0) and
nonnegative(op1) doesn't imply a nonzero addition.  Clearly op0 can
be negative and non-zero.


> +     case MAX_EXPR:
> +       if (tree_expr_nonzero_p (TREE_OPERAND (t, 0)))
> + 	{
> + 	  if (tree_expr_nonzero_p (TREE_OPERAND (t, 1)))
> + 	    return true;
> + 	  return (tree_expr_nonnegative_p (TREE_OPERAND (t, 0))
> + 	          && tree_expr_nonnegative_p (TREE_OPERAND (t, 1)));
> + 	}
> +       else if (tree_expr_nonzero_p (TREE_OPERAND (t, 1)))
> + 	{
> + 	  if (tree_expr_nonzero_p (TREE_OPERAND (t, 0)))
> + 	    return true;
> + 	  return (tree_expr_nonnegative_p (TREE_OPERAND (t, 0))
> + 	          && tree_expr_nonnegative_p (TREE_OPERAND (t, 1)));
> + 	}
> +       break;

For example, here I can't work out what you're trying to do.  Clearly
there's plenty of redundancy and unreachable clauses.  For the time
being you might consider the same implementation as MIN_EXPR whilst
you give the problem some thought.


> +     case RTL_EXPR:
> +       return nonzero_address_p (RTL_EXPR_RTL (t));

Again I think this only works if you know the RTL is being used to
perform an address calculation...


> +   else if (code == EQ_EXPR
> +            && integer_zerop (op1) && tree_expr_nonzero_p (op0))
> +     tem = build_int_2 (0, 0);
> +   else if (code == NE_EXPR
> +            && integer_zerop (op1) && tree_expr_nonzero_p (op0))
> +     tem = build_int_2 (1, 0);
> +

You need to check whether the type is floating point and we honor NaNs
in this mode for the second test.  In this case tree_expr_nonzero_p (op0)
can be true, but "op0 != 0" may still return false.

You can also use integer_zero_node and integer_one_node instead of
build_int_2.


Don't be in such a rush.  Clearly these changes are only suitable for
tree-ssa given the inherent complexity.  Best to think about the problem,
rewrite the patch, bootstrap, regression test and then resubmit.  Given
the duration of the review process, testing a patch before posting is
rarely if ever on the critical path.

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]