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] |

*From*: Roger Sayle <roger at eyesopen dot com>*To*: Jan Hubicka <jh at suse dot cz>*Cc*: Jan Hubicka <hubicka at ucw dot cz>, <gcc-patches at gcc dot gnu dot org>*Date*: Thu, 6 Nov 2003 09:11:43 -0700 (MST)*Subject*: 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 --

**Follow-Ups**:**Re: [tree-ssa/mainline PATCH] fold "test"==0 into constant***From:*Jan Hubicka

**References**:**Re: [tree-ssa/mainline PATCH] fold "test"==0 into constant***From:*Jan Hubicka

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |