On Tue, Sep 3, 2019 at 12:34 PM Ilya Leoshkevich <i...@linux.ibm.com> wrote:
>
> > Am 03.09.2019 um 12:07 schrieb Richard Biener <richard.guent...@gmail.com>:
> >
> > On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich <i...@linux.ibm.com> wrote:
> >>
> >>> Am 02.09.2019 um 12:37 schrieb Richard Biener
> >>> <richard.guent...@gmail.com>:
> >>>
> >>> On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <i...@linux.ibm.com>
> >>> wrote:
> >>>>
> >>>>> Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <i...@linux.ibm.com>:
> >>>>>
> >>>>>> Am 30.08.2019 um 09:12 schrieb Richard Biener
> >>>>>> <richard.guent...@gmail.com>:
> >>>>>>
> >>>>>> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <i...@linux.ibm.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <i...@linux.ibm.com>:
> >>>>>>>>
> >>>>>>>> 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.
