On October 12, 2019 10:44:06 AM GMT+02:00, Jakub Jelinek <ja...@redhat.com>
wrote:
>Hi!
>
>As mentioned in the PR and on IRC, tree_could_trap_p is described
>as taking GIMPLE expressions only, but in fact we rely on it not
>crashing
>when feeded GENERIC, just for GENERIC it will not handle expressions
>recursively and we have generic_expr_could_trap_p for that that calls
>tree_could_trap_p recursively.
>The addition of != COND_EXPR and != VEC_COND_EXPR assert to
>operation_could_trap_p broke this, because if GENERIC with COND_EXPR
>being first argument (condition) of another COND_EXPR is fed to
>tree_could_trap_p, it now ICEs.
>
>The following patch fixes it by:
>1) in tree_could_trap_p return false for {,VEC_}COND_EXPR rather than
>just recursing on the first argument. For GIMPLE, we shouldn't be
>called
>with {,VEC_}COND_EXPR, because those are ternary rhs and thus 3
>arguments,
>not one. For GENERIC, we can, but we also need to recurse and the
>recursion
>should discover that the comparison may trap.
>2) in simple_operand_p_2 which calls tree_could_trap_p on GENERIC, this
>is
>changed to generic_expr_could_trap_p so that it actually recurses and
>tests
>subexpressions too
>3) in operation_could_trap_helper_p signals that *COND_EXPR is not
>handled
>and it doesn't care about whether *COND_EXPR is floating point or not.
>This change is for sccvn, which calls operation_could_trap_helper_p
>and then, if not handled, calls tree_could_trap_p on the operands.
>Without the first hunk, *COND_EXPR is handled by the default handling,
>which says that fp_operation can trap (but *COND_EXPR with fp operands
>can't
>in itself) and makes it unhandled otherwise, at which point we call
>tree_could_trap_p on the condition, which is all that matters.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
>2019-10-12 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/92063
> * tree-eh.c (operation_could_trap_helper_p) <case COND_EXPR>
> <case VEC_COND_EXPR>: Return false with *handled = false.
> (tree_could_trap_p): For {,VEC_}COND_EXPR return false instead of
> recursing on the first operand.
> * fold-const.c (simple_operand_p_2): Use generic_expr_could_trap_p
> instead of tree_could_trap_p.
> * tree-ssa-sccvn.c (vn_nary_may_trap): Formatting fixes.
>
> * gcc.c-torture/compile/pr92063.c: New test.
>
>--- gcc/tree-eh.c.jj 2019-10-07 17:30:31.028153702 +0200
>+++ gcc/tree-eh.c 2019-10-11 13:46:12.626811039 +0200
>@@ -2499,6 +2499,14 @@ operation_could_trap_helper_p (enum tree
> /* Constructing an object cannot trap. */
> return false;
>
>+ case COND_EXPR:
>+ case VEC_COND_EXPR:
>+ /* Whether *COND_EXPR can trap depends on whether the
>+ first argument can trap, so signal it as not handled.
>+ Whether lhs is floating or not doesn't matter. */
>+ *handled = false;
>+ return false;
>+
> default:
> /* Any floating arithmetic may trap. */
> if (fp_operation && flag_trapping_math)
>@@ -2614,9 +2622,12 @@ tree_could_trap_p (tree expr)
> if (!expr)
> return false;
>
>- /* For COND_EXPR and VEC_COND_EXPR only the condition may trap. */
>+ /* In COND_EXPR and VEC_COND_EXPR only the condition may trap, but
>+ they won't appear as operands in GIMPLE form, so this is just for
>the
>+ GENERIC uses where it needs to recurse on the operands and so
>+ *COND_EXPR itself doesn't trap. */
>if (TREE_CODE (expr) == COND_EXPR || TREE_CODE (expr) == VEC_COND_EXPR)
>- expr = TREE_OPERAND (expr, 0);
>+ return false;
>
> code = TREE_CODE (expr);
> t = TREE_TYPE (expr);
>--- gcc/fold-const.c.jj 2019-10-09 10:27:12.578402783 +0200
>+++ gcc/fold-const.c 2019-10-11 12:29:53.426603712 +0200
>@@ -4447,8 +4447,7 @@ simple_operand_p_2 (tree exp)
> {
> enum tree_code code;
>
>- if (TREE_SIDE_EFFECTS (exp)
>- || tree_could_trap_p (exp))
>+ if (TREE_SIDE_EFFECTS (exp) || generic_expr_could_trap_p (exp))
> return false;
>
> while (CONVERT_EXPR_P (exp))
>--- gcc/tree-ssa-sccvn.c.jj 2019-09-24 14:39:07.479465423 +0200
>+++ gcc/tree-ssa-sccvn.c 2019-10-11 13:21:47.437326054 +0200
>@@ -5100,18 +5100,15 @@ vn_nary_may_trap (vn_nary_op_t nary)
> honor_nans = flag_trapping_math && !flag_finite_math_only;
> honor_snans = flag_signaling_nans != 0;
> }
>- else if (INTEGRAL_TYPE_P (type)
>- && TYPE_OVERFLOW_TRAPS (type))
>+ else if (INTEGRAL_TYPE_P (type) && TYPE_OVERFLOW_TRAPS (type))
> honor_trapv = true;
> }
> if (nary->length >= 2)
> rhs2 = nary->op[1];
> ret = operation_could_trap_helper_p (nary->opcode, fp_operation,
>- honor_trapv,
>- honor_nans, honor_snans, rhs2,
>- &handled);
>- if (handled
>- && ret)
>+ honor_trapv, honor_nans, honor_snans,
>+ rhs2, &handled);
>+ if (handled && ret)
> return true;
>
> for (i = 0; i < nary->length; ++i)
>--- gcc/testsuite/gcc.c-torture/compile/pr92063.c.jj 2019-10-11
>11:51:47.959149238 +0200
>+++ gcc/testsuite/gcc.c-torture/compile/pr92063.c 2019-10-11
>11:55:16.675090011 +0200
>@@ -0,0 +1,7 @@
>+/* PR middle-end/92063 */
>+
>+int
>+foo (int a, int b, int *c, short *d)
>+{
>+ return (c[0] ? b : 0) == 'y' && ((a ? d[0] : c[0]) ? b : 0) == 'c';
>+}
>
> Jakub
