From: Richard Biener <richard.guenther@gmail.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Richard Sandiford <richard.sandiford@arm.com>,
Segher Boessenkool <segher@kernel.crashing.org>,
"Joseph S. Myers" <joseph@codesourcery.com>,
Andreas Krebbel <krebbel@linux.ibm.com>,
Robin Dapp <rdapp@linux.ibm.com>
Subject: Re: [PATCH v3 1/9] Allow COND_EXPR and VEC_COND_EXPR condtions to trap
Date: Fri, 06 Sep 2019 11:07:00 -0000 [thread overview]
Message-ID: <CAFiYyc2k_andqHebmS8KATMZ3AWP=ch+3WzZK6do5DGvM3ifxg@mail.gmail.com> (raw)
In-Reply-To: <20190905111019.8951-2-iii@linux.ibm.com>
On Thu, Sep 5, 2019 at 1:10 PM Ilya Leoshkevich <iii@linux.ibm.com> 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 <iii@linux.ibm.com>
>
> 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
>
next prev parent reply other threads:[~2019-09-06 11:07 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-05 11:10 [PATCH v3 0/9] S/390: Use signaling FP comparison instructions Ilya Leoshkevich
2019-09-05 11:10 ` [PATCH v3 1/9] Allow COND_EXPR and VEC_COND_EXPR condtions to trap Ilya Leoshkevich
2019-09-06 11:07 ` Richard Biener [this message]
2019-09-06 15:45 ` Ilya Leoshkevich
2019-09-09 8:43 ` Richard Biener
2019-09-05 11:11 ` [PATCH v3 6/9] S/390: Remove code duplication in vec_unordered<mode> Ilya Leoshkevich
2019-09-30 14:41 ` Andreas Krebbel
2019-09-05 11:11 ` [PATCH v3 5/9] S/390: Implement vcond expander for V1TI,V1TF Ilya Leoshkevich
2019-09-30 14:51 ` Andreas Krebbel
2019-09-05 11:11 ` [PATCH v3 3/9] Introduce can_vector_compare_p function Ilya Leoshkevich
2019-09-06 12:58 ` Richard Sandiford
2019-09-05 11:11 ` [PATCH v3 2/9] Introduce rtx_alloca, alloca_raw_REG and alloca_rtx_fmt_* Ilya Leoshkevich
2019-09-06 11:09 ` Richard Biener
2019-09-06 12:40 ` Richard Sandiford
2019-09-30 15:00 ` Ilya Leoshkevich
2019-09-05 11:11 ` [PATCH v3 4/9] S/390: Do not use signaling vector comparisons on z13 Ilya Leoshkevich
2019-09-06 10:34 ` Segher Boessenkool
2019-09-30 13:36 ` Ilya Leoshkevich
2019-10-01 0:24 ` Segher Boessenkool
2019-09-05 11:12 ` [PATCH v3 9/9] S/390: Test signaling FP comparison instructions Ilya Leoshkevich
2019-09-05 11:12 ` [PATCH v3 7/9] S/390: Remove code duplication in vec_* comparison expanders Ilya Leoshkevich
2019-09-30 14:50 ` Andreas Krebbel
2019-09-05 11:12 ` [PATCH v3 8/9] S/390: Use signaling FP comparison instructions Ilya Leoshkevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc2k_andqHebmS8KATMZ3AWP=ch+3WzZK6do5DGvM3ifxg@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=iii@linux.ibm.com \
--cc=joseph@codesourcery.com \
--cc=krebbel@linux.ibm.com \
--cc=rdapp@linux.ibm.com \
--cc=richard.sandiford@arm.com \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).