> 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?
