From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 50275 invoked by alias); 6 Sep 2019 11:07:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 50264 invoked by uid 89); 6 Sep 2019 11:07:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=acts X-HELO: mail-lj1-f196.google.com Received: from mail-lj1-f196.google.com (HELO mail-lj1-f196.google.com) (209.85.208.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Sep 2019 11:07:24 +0000 Received: by mail-lj1-f196.google.com with SMTP id l20so5634376ljj.3 for ; Fri, 06 Sep 2019 04:07:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Sqo9+01t6HVq6FHTo4GrAH+EQD725ZklCPG96TvG2pY=; b=i1Dl7ONSLT/4BdKW/hiGvH/fHdBc4zPGNmiY6KiFobfAn0Bb7354U/I/nAkJE+D7Xg xZ5hqbBQHK6vtY/FtNd0P3ISBThLd+oyEfdFxHcfe0oIOP3mHNosrBVnqWgtR29Rsmip NI6B+CXtOF8iNHe74IUbn3XWUTa/0EptbG+QDbaXNSi6QmIUq536dhkS0n+QGdyQbhBb 1PUdKhwi7/AKcnh/AtBKr+HFGDgzy1Y8HUfbCvgU9gUKqEjV/TZSPVMcPkW5D5DY9Dnn r3E4jVU3/Fj2bLXiNT4vMcR8XzMF93BhsuAkcohf/U1aZSJxOCJF8TEwEfpLY4mSWU2t CpFA== MIME-Version: 1.0 References: <20190905111019.8951-1-iii@linux.ibm.com> <20190905111019.8951-2-iii@linux.ibm.com> In-Reply-To: <20190905111019.8951-2-iii@linux.ibm.com> From: Richard Biener Date: Fri, 06 Sep 2019 11:07:00 -0000 Message-ID: Subject: Re: [PATCH v3 1/9] Allow COND_EXPR and VEC_COND_EXPR condtions to trap To: Ilya Leoshkevich Cc: GCC Patches , Richard Sandiford , Segher Boessenkool , "Joseph S. Myers" , Andreas Krebbel , Robin Dapp Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00363.txt.bz2 On Thu, Sep 5, 2019 at 1:10 PM Ilya Leoshkevich wrote: > > Right now gimplifier does not allow VEC_COND_EXPR's condition to trap > and introduces a temporary if this could happen, for example, generating > > _5 = _4 > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }; > _6 = VEC_COND_EXPR <_5, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>; > > from GENERIC > > VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) , > { -1, -1, -1, -1 } , > { 0, 0, 0, 0 } > > > This is not necessary and makes the resulting GIMPLE harder to analyze. > In particular, one of the next patches in series needs to get to > VEC_COND_EXPR's comparison code, which is not possible when a temporary > is introduced. > > This patch takes special care to avoid introducing trapping comparisons > in GIMPLE_COND. They are not allowed, because they would require 3 > outgoing edges (then, else and EH), which is awkward to say the least. > Therefore, computations of such conditions should live in their own basic > blocks. Comments inline (thanks for the work btw) > gcc/ChangeLog: > > 2019-09-03 Ilya Leoshkevich > > PR target/77918 > * gimple-expr.c (gimple_cond_get_ops_from_tree): Assert that the > caller passes a non-trapping condition. > (is_gimple_condexpr): Allow trapping conditions. > (is_gimple_condexpr_1): New helper function. > (is_gimple_condexpr_for_cond): New function, acts like old > is_gimple_condexpr. > * gimple-expr.h (is_gimple_condexpr_for_cond): New function. > (gimple_ternary_operands_ok_p): New function to remove code > duplication i verify_gimple_assign_ternary and > valid_gimple_rhs_p. > * gimple.c (gimple_could_trap_p_1): Handle COND_EXPR and > VEC_COND_EXPR. > * gimplify.c (gimplify_cond_expr): Use > is_gimple_condexpr_for_cond. > (gimplify_expr): Allow is_gimple_condexpr_for_cond. > * tree-cfg.c (verify_gimple_assign_ternary): Use > gimple_ternary_operands_ok_p. > * tree-eh.c (operation_could_trap_p): Assert on COND_EXPR and > VEC_COND_EXPR. > (tree_could_trap_p): Handle COND_EXPR and VEC_COND_EXPR. > * tree-ssa-forwprop.c (forward_propagate_into_gimple_cond): Use > is_gimple_condexpr_for_cond, remove pointless tmp check > (forward_propagate_into_cond): Remove pointless tmp check. > * tree-ssa-propagate.c (valid_gimple_rhs_p): Use > gimple_ternary_operands_ok_p. > --- > gcc/gimple-expr.c | 25 +++++++++++++++++++++---- > gcc/gimple-expr.h | 11 +++++++++++ > gcc/gimple.c | 2 ++ > gcc/gimplify.c | 5 +++-- > gcc/tree-cfg.c | 5 +---- > gcc/tree-eh.c | 8 ++++++++ > gcc/tree-ssa-forwprop.c | 7 ++++--- > gcc/tree-ssa-propagate.c | 9 ++++----- > 8 files changed, 54 insertions(+), 18 deletions(-) > > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > index b0c9f9b671a..46e5f000580 100644 > --- a/gcc/gimple-expr.c > +++ b/gcc/gimple-expr.c > @@ -571,6 +571,7 @@ gimple_cond_get_ops_from_tree (tree cond, enum tree_code *code_p, > || TREE_CODE (cond) == TRUTH_NOT_EXPR > || is_gimple_min_invariant (cond) > || SSA_VAR_P (cond)); > + gcc_assert (!tree_could_throw_p (cond)); make this gcc_checking_assert () > extract_ops_from_tree (cond, code_p, lhs_p, rhs_p); > > @@ -602,17 +603,33 @@ is_gimple_lvalue (tree t) > || TREE_CODE (t) == BIT_FIELD_REF); > } > > -/* Return true if T is a GIMPLE condition. */ > +/* Helper for is_gimple_condexpr and is_gimple_condexpr_for_cond. */ > > -bool > -is_gimple_condexpr (tree t) > +static bool > +is_gimple_condexpr_1 (tree t, bool allow_traps) > { > return (is_gimple_val (t) || (COMPARISON_CLASS_P (t) > - && !tree_could_throw_p (t) > + && (allow_traps || !tree_could_throw_p (t)) > && is_gimple_val (TREE_OPERAND (t, 0)) > && is_gimple_val (TREE_OPERAND (t, 1)))); > } > > +/* Return true if T is a GIMPLE condition. */ > + > +bool > +is_gimple_condexpr (tree t) > +{ > + return is_gimple_condexpr_1 (t, true); > +} > + > +/* Like is_gimple_condexpr, but does not allow T to trap. */ > + > +bool > +is_gimple_condexpr_for_cond (tree t) > +{ > + return is_gimple_condexpr_1 (t, false); > +} > + > /* Return true if T is a gimple address. */ > > bool > diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h > index 1ad1432bd17..27190b1e5fe 100644 > --- a/gcc/gimple-expr.h > +++ b/gcc/gimple-expr.h > @@ -41,6 +41,7 @@ extern void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *, > tree *); > extern bool is_gimple_lvalue (tree); > extern bool is_gimple_condexpr (tree); > +extern bool is_gimple_condexpr_for_cond (tree); > extern bool is_gimple_address (const_tree); > extern bool is_gimple_invariant_address (const_tree); > extern bool is_gimple_ip_invariant_address (const_tree); > @@ -175,4 +176,14 @@ gimple_call_addr_fndecl (const_tree fn) > return NULL_TREE; > } > > +static inline bool > +gimple_ternary_operands_ok_p (enum tree_code code, tree op1, tree op2, tree op3) > +{ > + return ((code == VEC_COND_EXPR || code == COND_EXPR) > + ? is_gimple_condexpr (op1) > + : is_gimple_val (op1)) > + && is_gimple_val (op2) > + && is_gimple_val (op3); > +} Hmm, I don't like this too much, please simply adjust the two places you use it (there's actually nothign to adjust?) > #endif /* GCC_GIMPLE_EXPR_H */ > diff --git a/gcc/gimple.c b/gcc/gimple.c > index 633ef512a19..fd14fbec15e 100644 > --- a/gcc/gimple.c > +++ b/gcc/gimple.c > @@ -2144,6 +2144,8 @@ gimple_could_trap_p_1 (gimple *s, bool include_mem, bool include_stores) > op = gimple_assign_rhs_code (s); > if (get_gimple_rhs_class (op) == GIMPLE_BINARY_RHS) > div = gimple_assign_rhs2 (s); > + else if (op == COND_EXPR || op == VEC_COND_EXPR) > + op = TREE_CODE (gimple_assign_rhs1 (s)); I think this is not correct since we can have int i = fp > 1. ? intval1 : intval2 and thus FLOAT_TYPE_P (t) is wrong. You need to do t = TREE_TYPE (op); as well I think. OK with those changes - you can apply this independently of the rest of the series btw. Thanks, Richard. > return (operation_could_trap_p (op, FLOAT_TYPE_P (t), > (INTEGRAL_TYPE_P (t) > && TYPE_OVERFLOW_TRAPS (t)), > diff --git a/gcc/gimplify.c b/gcc/gimplify.c > index daa0b71c191..920e423381c 100644 > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -4141,8 +4141,8 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback) > /* Now do the normal gimplification. */ > > /* Gimplify condition. */ > - ret = gimplify_expr (&TREE_OPERAND (expr, 0), pre_p, NULL, is_gimple_condexpr, > - fb_rvalue); > + ret = gimplify_expr (&TREE_OPERAND (expr, 0), pre_p, NULL, > + is_gimple_condexpr_for_cond, fb_rvalue); > if (ret == GS_ERROR) > return GS_ERROR; > gcc_assert (TREE_OPERAND (expr, 0) != NULL_TREE); > @@ -12973,6 +12973,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, > else if (gimple_test_f == is_gimple_val > || gimple_test_f == is_gimple_call_addr > || gimple_test_f == is_gimple_condexpr > + || gimple_test_f == is_gimple_condexpr_for_cond > || gimple_test_f == is_gimple_mem_rhs > || gimple_test_f == is_gimple_mem_rhs_or_call > || gimple_test_f == is_gimple_reg_rhs > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index b75fdb2e63f..4d2239e9d2f 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -4121,10 +4121,7 @@ verify_gimple_assign_ternary (gassign *stmt) > return true; > } > > - if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR) > - ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1)) > - || !is_gimple_val (rhs2) > - || !is_gimple_val (rhs3)) > + if (!gimple_ternary_operands_ok_p (rhs_code, rhs1, rhs2, rhs3)) > { > error ("invalid operands in ternary operation"); > return true; > diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c > index 5bb07e49d28..98dc95a07cd 100644 > --- a/gcc/tree-eh.c > +++ b/gcc/tree-eh.c > @@ -2523,6 +2523,10 @@ operation_could_trap_p (enum tree_code op, bool fp_operation, bool honor_trapv, > bool honor_snans = fp_operation && flag_signaling_nans != 0; > bool handled; > > + /* This function cannot tell whether or not COND_EXPR and VEC_COND_EXPR could > + trap, because that depends on the respective condition op. */ > + gcc_assert (op != COND_EXPR && op != VEC_COND_EXPR); > + > if (TREE_CODE_CLASS (op) != tcc_comparison > && TREE_CODE_CLASS (op) != tcc_unary > && TREE_CODE_CLASS (op) != tcc_binary) > @@ -2610,6 +2614,10 @@ tree_could_trap_p (tree expr) > if (!expr) > return false; > > + /* For COND_EXPR and VEC_COND_EXPR, only the condition may trap. */ > + if (TREE_CODE (expr) == COND_EXPR || TREE_CODE (expr) == VEC_COND_EXPR) > + expr = TREE_OPERAND (expr, 0); > + > code = TREE_CODE (expr); > t = TREE_TYPE (expr); > > diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c > index c464c899586..edc5417f455 100644 > --- a/gcc/tree-ssa-forwprop.c > +++ b/gcc/tree-ssa-forwprop.c > @@ -524,9 +524,10 @@ forward_propagate_into_gimple_cond (gcond *stmt) > tmp = forward_propagate_into_comparison_1 (stmt, code, > boolean_type_node, > rhs1, rhs2); > - if (tmp) > + if (tmp > + && is_gimple_condexpr_for_cond (tmp)) > { > - if (dump_file && tmp) > + if (dump_file) > { > fprintf (dump_file, " Replaced '"); > print_gimple_expr (dump_file, stmt, 0); > @@ -604,7 +605,7 @@ forward_propagate_into_cond (gimple_stmt_iterator *gsi_p) > if (tmp > && is_gimple_condexpr (tmp)) > { > - if (dump_file && tmp) > + if (dump_file) > { > fprintf (dump_file, " Replaced '"); > print_generic_expr (dump_file, cond); > diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c > index 7172ef8b4e6..be34e25fc40 100644 > --- a/gcc/tree-ssa-propagate.c > +++ b/gcc/tree-ssa-propagate.c > @@ -523,11 +523,10 @@ valid_gimple_rhs_p (tree expr) > default: > if (get_gimple_rhs_class (code) == GIMPLE_TERNARY_RHS) > { > - if (((code == VEC_COND_EXPR || code == COND_EXPR) > - ? !is_gimple_condexpr (TREE_OPERAND (expr, 0)) > - : !is_gimple_val (TREE_OPERAND (expr, 0))) > - || !is_gimple_val (TREE_OPERAND (expr, 1)) > - || !is_gimple_val (TREE_OPERAND (expr, 2))) > + if (!gimple_ternary_operands_ok_p (code, > + TREE_OPERAND (expr, 0), > + TREE_OPERAND (expr, 1), > + TREE_OPERAND (expr, 2))) > return false; > break; > } > -- > 2.21.0 >