From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95139 invoked by alias); 3 Sep 2019 11:29:54 -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 95131 invoked by uid 89); 3 Sep 2019 11:29:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= 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; Tue, 03 Sep 2019 11:29:52 +0000 Received: by mail-lj1-f196.google.com with SMTP id l1so15605064lji.12 for ; Tue, 03 Sep 2019 04:29:51 -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=YC3XmsjM39ybZSWhuqi7fxZ/4okNqiS75CSYbEY0/hU=; b=FF4Ys3o+SyIH80i90uFmuthOf6PC52lmVC7PJr3MLZIhV8qqbtka2Vl+3oBcr25tpR PfDduvwOsAMY0PEX0nqENOOV3CpFGddtnXASceKKCzkra5VATJ154CDUmj+RrSJJpdnf TcFFfMq3+CS4T4EJLpJRKDlIhdW3rZ/jbYj2D5oZOqQwny3eYDh9k+SCLTAY4us9fw8+ CO91jDlmqbEf33br2VqKHKrjbMZ03peXwxIstSzxcs8REHmQ/1G/3YwvQvQMw0r/QYET WOES+dRy9Zgnrg3IKLcCtLLRdKYZQy8e02QOPYdU4QA6EsYXC1a3yxZXAhR77sHvxW5S NAUQ== MIME-Version: 1.0 References: <20190822134551.18924-1-iii@linux.ibm.com> <1A6BDF56-CD21-4D85-A95C-9EA3FEC6D342@linux.ibm.com> <1CB9DB68-37CC-4684-A6F3-7A1D693FC391@linux.ibm.com> In-Reply-To: From: Richard Biener Date: Tue, 03 Sep 2019 11:29:00 -0000 Message-ID: Subject: Re: [PATCH v2 0/9] S/390: Use signaling FP comparison instructions To: Ilya Leoshkevich Cc: GCC Patches , Richard Sandiford , Segher Boessenkool Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00106.txt.bz2 On Tue, Sep 3, 2019 at 12:34 PM Ilya Leoshkevich wrote: > > > Am 03.09.2019 um 12:07 schrieb Richard Biener : > > > > On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich wrote: > >> > >>> Am 02.09.2019 um 12:37 schrieb Richard Biener : > >>> > >>> On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich wrote: > >>>> > >>>>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich : > >>>>> > >>>>>> Am 30.08.2019 um 09:12 schrieb Richard Biener : > >>>>>> > >>>>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich wrote: > >>>>>>> > >>>>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich : > >>>>>>>> > >>>>>>>> Bootstrap and regtest running on x86_64-redhat-linux and > >>>>>>>> s390x-redhat-linux. > >>>>>>>> > >>>>>>>> This patch series adds signaling FP comparison support (both scalar and > >>>>>>>> vector) to s390 backend. > >>>>>>> > >>>>>>> I'm running into a problem on ppc64 with this patch, and it would be > >>>>>>> great if someone could help me figure out the best way to resolve it. > >>>>>>> > >>>>>>> vector36.C test is failing because gimplifier produces the following > >>>>>>> > >>>>>>> _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 > >>>>>>> > >>>>>>> VEC_COND_EXPR < (*b > { 2.0e+0, 2.0e+0, 2.0e+0, 2.0e+0 }) , > >>>>>>> { -1, -1, -1, -1 } , > >>>>>>> { 0, 0, 0, 0 } > > >>>>>>> > >>>>>>> Since the comparison tree code is now hidden behind a temporary, my code > >>>>>>> does not have anything to pass to the backend. The reason for creating > >>>>>>> a temporary is that the comparison can trap, and so the following check > >>>>>>> in gimplify_expr fails: > >>>>>>> > >>>>>>> if (gimple_seq_empty_p (internal_post) && (*gimple_test_f) (*expr_p)) > >>>>>>> goto out; > >>>>>>> > >>>>>>> gimple_test_f is is_gimple_condexpr, and it eventually calls > >>>>>>> operation_could_trap_p (GT). > >>>>>>> > >>>>>>> My current solution is to simply state that backend does not support > >>>>>>> SSA_NAME in vector comparisons, however, I don't like it, since it may > >>>>>>> cause performance regressions due to having to fall back to scalar > >>>>>>> comparisons. > >>>>>>> > >>>>>>> I was thinking about two other possible solutions: > >>>>>>> > >>>>>>> 1. Change the gimplifier to allow trapping vector comparisons. That's > >>>>>>> a bit complicated, because tree_could_throw_p checks not only for > >>>>>>> floating point traps, but also e.g. for array index out of bounds > >>>>>>> traps. So I would have to create a tree_could_throw_p version which > >>>>>>> disregards specific kinds of traps. > >>>>>>> > >>>>>>> 2. Change expand_vector_condition to follow SSA_NAME_DEF_STMT and use > >>>>>>> its tree_code instead of SSA_NAME. The potential problem I see with > >>>>>>> this is that there appears to be no guarantee that _5 will be inlined > >>>>>>> into _6 at a later point. So if we say that we don't need to fall > >>>>>>> back to scalar comparisons based on availability of vector > > >>>>>>> instruction and inlining does not happen, then what's actually will > >>>>>>> be required is vector selection (vsel on S/390), which might not be > >>>>>>> available in general case. > >>>>>>> > >>>>>>> What would be a better way to proceed here? > >>>>>> > >>>>>> On GIMPLE there isn't a good reason to split out trapping comparisons > >>>>>> from [VEC_]COND_EXPR - the gimplifier does this for GIMPLE_CONDs > >>>>>> where it is important because we'd have no way to represent EH info > >>>>>> when not done. It might be a bit awkward to preserve EH across RTL > >>>>>> expansion though in case the [VEC_]COND_EXPR are not expanded > >>>>>> as a single pattern, but I'm not sure. > >>>>> > >>>>> Ok, so I'm testing the following now - for the problematic test that > >>>>> helped: > >>>>> > >>>>> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > >>>>> index b0c9f9b671a..940aa394769 100644 > >>>>> --- a/gcc/gimple-expr.c > >>>>> +++ b/gcc/gimple-expr.c > >>>>> @@ -602,17 +602,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_possibly_trapping_gimple_condexpr. */ > >>>>> > >>>>> -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, false); > >>>>> +} > >>>>> + > >>>>> +/* Like is_gimple_condexpr, but allow the T to trap. */ > >>>>> + > >>>>> +bool > >>>>> +is_possibly_trapping_gimple_condexpr (tree t) > >>>>> +{ > >>>>> + return is_gimple_condexpr_1 (t, true); > >>>>> +} > >>>>> + > >>>>> /* Return true if T is a gimple address. */ > >>>>> > >>>>> bool > >>>>> diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h > >>>>> index 1ad1432bd17..20546ca5b99 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_possibly_trapping_gimple_condexpr (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); > >>>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c > >>>>> index daa0b71c191..4e6256390c0 100644 > >>>>> --- a/gcc/gimplify.c > >>>>> +++ b/gcc/gimplify.c > >>>>> @@ -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_possibly_trapping_gimple_condexpr > >>>>> || gimple_test_f == is_gimple_mem_rhs > >>>>> || gimple_test_f == is_gimple_mem_rhs_or_call > >>>>> || gimple_test_f == is_gimple_reg_rhs > >>>>> @@ -13814,7 +13815,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, > >>>>> enum gimplify_status r0, r1, r2; > >>>>> > >>>>> r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > >>>>> - post_p, is_gimple_condexpr, fb_rvalue); > >>>>> + post_p, is_possibly_trapping_gimple_condexpr, fb_rvalue); > >>>>> r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p, > >>>>> post_p, is_gimple_val, fb_rvalue); > >>>>> r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p, > >>>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > >>>>> index b75fdb2e63f..175b858f56b 100644 > >>>>> --- a/gcc/tree-cfg.c > >>>>> +++ b/gcc/tree-cfg.c > >>>>> @@ -4121,8 +4121,11 @@ 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)) > >>>>> + if ((rhs_code == VEC_COND_EXPR > >>>>> + ? !is_possibly_trapping_gimple_condexpr (rhs1) > >>>>> + : (rhs_code == COND_EXPR > >>>>> + ? !is_gimple_condexpr (rhs1) > >>>>> + : !is_gimple_val (rhs1))) > >>>>> || !is_gimple_val (rhs2) > >>>>> || !is_gimple_val (rhs3)) > >>>>> { > >>>>> > >>>>>> > >>>>>> To go this route you'd have to split the is_gimple_condexpr check > >>>>>> I guess and eventually users turning [VEC_]COND_EXPR into conditional > >>>>>> code (do we have any?) have to be extra careful then. > >>>>>> > >>>>> > >>>>> We have expand_vector_condition, which turns VEC_COND_EXPR into > >>>>> COND_EXPR - but this should be harmless, right? I could not find > >>>>> anything else. > >>>> > >>>> Ugh, I've realized I need to check not only VEC_COND_EXPR, but also > >>>> COND_EXPR usages. There is, of course, a great deal more code, so I'm > >>>> not sure whether I looked exhaustively through it, but there are at > >>>> least store_expr and do_jump which do exactly this during expansion. > >>>> Should we worry about EH edges at this point? > >>> > >>> Well, the EH edge needs to persist (and be rooted off the comparison, > >>> not the selection). > >> > >> Ok, I'm trying to create some samples that may reveal problems with EH > >> edges in these two cases. So far with these experiments I only managed > >> to find and unrelated S/390 bug :-) > >> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00065.html > >> > >>> That said, I'd simply stop using is_gimple_condexpr for GIMPLE_CONDs > >>> (it allows is_gimple_val which isn't proper form for GIMPLE_COND). Of course > >>> there's code using it for GIMPLE_CONDs which would need to be adjusted. > >> > >> I'm sorry, I don't quite get this - what would that buy us? and what > >> would you use instead? Right now we fix up non-conforming values > >> accepted by is_gimple_val using gimple_cond_get_ops_from_tree - is > >> there a problem with this approach? > >> > >> What I have in mind right now is to: > >> - allow trapping conditions for COND_EXPR and VEC_COND_EXPR; > >> - report them as trapping in operation_could_trap_p and > >> tree_could_trap_p iff their condition is trapping; > >> - find and adjust all places where this messes up EH edges. > >> > >> GIMPLE_COND logic appears to be already covered precisely because it > >> uses is_gimple_condexpr. > >> > >> Am I missing something? > > > > Not really - all I'm saying is that currently we use is_gimple_condexpr > > to check whether a GENERIC tree is suitable for [VEC_]COND_EXPR > > during for example forward propagation. > > > > And GIMPLE_COND already uses its own logic (as you say) but > > still passes use is_gimple_condexpr for it. > > > > So my proposal would be to change is_gimple_condexpr to > > allow trapping [VEC_]COND_EXPR and stop using is_gimple_condexpr > > checks on conditions to be used for GIMPLE_CONDs (and substitute > > another predicate there). For this to work and catch wrong-doings > > we should amend gimple_cond_get_ops_from_tree to assert > > that the extracted condition cannot trap. > > Ah, I think now I understand. While I wanted to keep is_gimple_condexpr > as is and introduce is_possibly_trapping_gimple_condexpr, you're saying > we rather need to change is_gimple_condexpr and introduce, say, > is_non_trapping_gimple_condexpr. > > This makes sense, thanks for the explanation! I'd say is_gimple_cond_expr - bah! stupid clashing names ;) OK, so is_gimple_cond_condexpr vs. is_gimple_condexpr_cond? Hmm, no. I don't like to explicitely spell "non_trapping" here but instead find names that tell one is for GIMPLE_COND while the other is for GIMPLE_ASSIGN with a [VEC_]COND_EXPR operation. Ah, maybe is_gimple_cond_condition () vs. is_gimple_assign_condition ()? Or is_gimple_condexpr_for_cond and is_gimple_condexpr_for_assign? Richard.