Re: [PATCH v2 0/9] S/390: Use signaling FP comparison instructions

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Aug 30, 2019 at 5:25 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 30.08.2019 um 16:40 schrieb Ilya Leoshkevich <iii@linux.ibm.com>:
> >
> >> Am 30.08.2019 um 09:12 schrieb Richard Biener <richard.guenther@gmail.com>:
> >>
> >> On Thu, Aug 29, 2019 at 5:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>>
> >>>> Am 22.08.2019 um 15:45 schrieb Ilya Leoshkevich <iii@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).

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.

Richard.