This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH v2 0/9] S/390: Use signaling FP comparison instructions
> 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.