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

[Ilya Leoshkevich <iii at linux dot ibm dot com>]

> Am 03.09.2019 um 12:07 schrieb Richard Biener <richard.guenther@gmail.com>:
> 
> On Mon, Sep 2, 2019 at 6:28 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>>> Am 02.09.2019 um 12:37 schrieb 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).
>> 
>> 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!