On Fri, Nov 12, 2021 at 07:30:35AM +0000, Tamar Christina via Gcc-patches wrote:
> @@ -2099,7 +2124,7 @@ spaceship_replacement (basic_block cond_bb, basic_block
> middle_bb,
> || !tree_fits_shwi_p (rhs)
> || !IN_RANGE (tree_to_shwi (rhs), -1, 1))
> return false;
> - if (orig_use_lhs)
> + if (orig_use_lhs && !is_cast)
> {
> if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
> return false;
I actually meant that you'd do the if (is_cast) handling right above
the if (orig_use_lhs), i.e.
if (is_cast)
{
if (TREE_CODE (rhs) != INTEGER_CST)
return false;
/* As for -ffast-math we assume the 2 return to be
impossible, canonicalize (unsigned) res <= 1U or
(unsigned) res < 2U into res >= 0 and (unsigned) res > 1U
or (unsigned) res >= 2U as res < 0. */
switch (cmp)
{
case LE_EXPR:
if (!integer_onep (rhs))
return false;
cmp = GE_EXPR;
break;
case LT_EXPR:
if (wi::ne_p (wi::to_widest (rhs), 2))
return false;
cmp = GE_EXPR;
break;
case GT_EXPR:
if (!integer_onep (rhs))
return false;
cmp = LT_EXPR;
break;
case GE_EXPR:
if (wi::ne_p (wi::to_widest (rhs), 2))
return false;
cmp = LT_EXPR;
break;
default:
return false;
}
rhs = build_zero_cst (TREE_TYPE (phires));
}
else if (orig_use_lhs)
...
and keep the code in the following hunk untouched. Similarly to how
for the BIT_AND_EXPR if (orig_use_lhs), it virtually undoes the match.pd
optimization.
Because in the place you've placed it you're totally ignoring one_cmp,
and I'm pretty sure that is the wrong thing.
one_cmp is computed as:
/* lhs1 one_cmp rhs1 results in phires of 1. */
enum tree_code one_cmp;
if ((cmp1 == LT_EXPR || cmp1 == LE_EXPR)
^ (!integer_onep ((e1->flags & EDGE_TRUE_VALUE) ? arg1 : arg0)))
one_cmp = LT_EXPR;
else
one_cmp = GT_EXPR;
and it is something unrelated to what actual comparison is done or virtually
done on the phires.
> @@ -2310,62 +2335,101 @@ spaceship_replacement (basic_block cond_bb,
> basic_block middle_bb,
> one_cmp = GT_EXPR;
>
> enum tree_code res_cmp;
> - switch (cmp)
> +
> + if (is_cast)
> {
> - case EQ_EXPR:
> - if (integer_zerop (rhs))
> - res_cmp = EQ_EXPR;
> - else if (integer_minus_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> - else if (integer_onep (rhs))
> - res_cmp = one_cmp;
> - else
> + if (TREE_CODE (rhs) != INTEGER_CST)
> return false;
> - break;
> - case NE_EXPR:
> - if (integer_zerop (rhs))
> - res_cmp = NE_EXPR;
> - else if (integer_minus_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> - else if (integer_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> - else
> - return false;
> - break;
> - case LT_EXPR:
> - if (integer_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> - else if (integer_zerop (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> - else
> - return false;
> - break;
> - case LE_EXPR:
> - if (integer_zerop (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> - else if (integer_minus_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> - else
> - return false;
> - break;
> - case GT_EXPR:
> - if (integer_minus_onep (rhs))
> - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> - else if (integer_zerop (rhs))
> - res_cmp = one_cmp;
> - else
> - return false;
> - break;
> - case GE_EXPR:
> - if (integer_zerop (rhs))
> - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> - else if (integer_onep (rhs))
> - res_cmp = one_cmp;
> - else
> - return false;
> - break;
> - default:
> - gcc_unreachable ();
> + /* As for -ffast-math we assume the 2 return to be
> + impossible, canonicalize (unsigned) res <= 1U or
> + (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U
> + or (unsigned) res >= 2U as res < 0. */
> + switch (cmp)
> + {
> + case LE_EXPR:
> + if (!integer_onep (rhs))
> + return false;
> + res_cmp = GE_EXPR;
> + break;
> + case LT_EXPR:
> + if (wi::ne_p (wi::to_widest (rhs), 2))
> + return false;
> + res_cmp = GE_EXPR;
> + break;
> + case GT_EXPR:
> + if (!integer_onep (rhs))
> + return false;
> + res_cmp = LT_EXPR;
> + break;
> + case GE_EXPR:
> + if (wi::ne_p (wi::to_widest (rhs), 2))
> + return false;
> + res_cmp = LT_EXPR;
> + break;
> + default:
> + return false;
> + }
> + rhs = build_zero_cst (TREE_TYPE (phires));
> + }
> + else
> + {
> + switch (cmp)
> + {
> + case EQ_EXPR:
> + if (integer_zerop (rhs))
> + res_cmp = EQ_EXPR;
> + else if (integer_minus_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> + else if (integer_onep (rhs))
> + res_cmp = one_cmp;
> + else
> + return false;
> + break;
> + case NE_EXPR:
> + if (integer_zerop (rhs))
> + res_cmp = NE_EXPR;
> + else if (integer_minus_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> + else if (integer_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> + else
> + return false;
> + break;
> + case LT_EXPR:
> + if (integer_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> + else if (integer_zerop (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> + else
> + return false;
> + break;
> + case LE_EXPR:
> + if (integer_zerop (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
> + else if (integer_minus_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> + else
> + return false;
> + break;
> + case GT_EXPR:
> + if (integer_minus_onep (rhs))
> + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> + else if (integer_zerop (rhs))
> + res_cmp = one_cmp;
> + else
> + return false;
> + break;
> + case GE_EXPR:
> + if (integer_zerop (rhs))
> + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> + else if (integer_onep (rhs))
> + res_cmp = one_cmp;
> + else
> + return false;
> + break;
> + default:
> + gcc_unreachable ();
> + }
> }
>
> if (gimple_code (use_stmt) == GIMPLE_COND)
> @@ -2405,12 +2470,14 @@ spaceship_replacement (basic_block cond_bb,
> basic_block middle_bb,
> }
> if (orig_use_lhs)
> {
> - if (!has_debug_uses)
> + if (!has_debug_uses || is_cast)
> FOR_EACH_IMM_USE_FAST (use_p, iter, orig_use_lhs)
> {
> gimple *use_stmt = USE_STMT (use_p);
> gcc_assert (is_gimple_debug (use_stmt));
> has_debug_uses = true;
> + if (is_cast)
> + has_cast_debug_uses = true;
> }
> gimple_stmt_iterator gsi = gsi_for_stmt (orig_use_stmt);
> tree zero = build_zero_cst (TREE_TYPE (orig_use_lhs));
> @@ -2448,7 +2515,23 @@ spaceship_replacement (basic_block cond_bb,
> basic_block middle_bb,
> gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> replace_uses_by (phires, temp2);
> if (orig_use_lhs)
> - replace_uses_by (orig_use_lhs, temp2);
> + {
> + if (has_cast_debug_uses)
> + {
> + tree temp3 = make_node (DEBUG_EXPR_DECL);
> + DECL_ARTIFICIAL (temp3) = 1;
> + TREE_TYPE (temp3) = TREE_TYPE (orig_use_lhs);
> + SET_DECL_MODE (temp3, TYPE_MODE (type));
> + t = build2 (EQ_EXPR, boolean_type_node, lhs1, rhs2);
> + t = build3 (COND_EXPR, type, t, build_zero_cst (type),
> + temp1);
This will create a debug stmt with correct type on lhs, but
incorrect on the rhs (type rather than TREE_TYPE (orig_use_lhs).
You should instead of the above 3 lines do:
t = fold_convert (TREE_TYPE (temp3), temp2);
Otherwise LGTM.
Jakub
