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


> > +     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.

We use it to fold EXP!=0 to true when EXP is nonzero.  This would fail
for NaNs.  Can I check for this?
> 
> 
> > +
> > +     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 :>

I noticed that already and have patch in testing, just didn't want to
send it too early again..
> 
> 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 was thinking about this too, problem is that in each use I need
tree_expr_positive_p I already know that the operand is either
nonnegative or nonzero.  That means that I create unnecesary recursion
that may get expensive for deep expressions.

Again, if it is your's preference (and code is really so nasty to
review), I will change this easilly.  It is not that big deal, but
perhaps adding comments would make code reachable without extra
overhead...
> 
> 
> > +     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.

I am testing that either both operands are nonzero, or one of them is
nonzero and both are positive.  What clause in unreahcable?
> 
> 
> > +     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...
nonzero_address_p already checks that it is doing address calculation.
This is done in broken RTL way where we lost types on PLUS expression,
so perhaps we can simply drop this case.  It is no longer interesting
anyway.  Sounds OK?
> 
> 
> > +   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.

I would expect that defining tree_expr_nonzero_p in a way that it
returns false on nans will check at least trivial FP cases.  Hmm,
probably just constants that are not interesting anyway.  So perhaps we
can limit this to integer modes only?
> 
> You can also use integer_zero_node and integer_one_node instead of
> build_int_2.

I can not.  The function is written in a way so it later overwrite the
TREE_TYPE destructivly.
> 
> 
> 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.

Sorry for that.
THis time I will check properly.
Honza
> 
> Roger
> --


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