On Wed, Nov 03, 2021 at 10:56:30AM +0000, Tamar Christina wrote:
> The spaceship operator is looking for (res & 1) == res which was previously
> folded to (res & ~1) == 0.
> This is now being folded further to ((unsigned) res) <= 1.
Is that match.pd change already in the tree (which commit) or not yet?
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -2038,6 +2038,26 @@ spaceship_replacement (basic_block cond_bb,
> basic_block middle_bb,
> gimple *orig_use_stmt = use_stmt;
> tree orig_use_lhs = NULL_TREE;
> int prec = TYPE_PRECISION (TREE_TYPE (phires));
> +
> + /* Deal with if match.pd has rewritten the (res & ~1) == 0
> + into res <= 1 and has left a type-cast for signed types. */
The above sentence doesn't make sense gramatically. Either
Deal with match.pd rewriting ... and leaving ... or
Deal with the case when match.pd ...
or something similar?
> + if (gimple_assign_cast_p (use_stmt))
> + {
> + orig_use_lhs = gimple_assign_lhs (use_stmt);
> + /* Match.pd would have only done this for a signed type,
> + so the conversion must be to an unsigned one. */
Even at the start of sentence it should be match.pd, the file isn't
called Match.pd.
> + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)))
> + return false;
> + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
> + return false;
> + if (EDGE_COUNT (phi_bb->preds) != 4)
> + return false;
> + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)))
> + return false;
You are testing !TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)) twice, did you
mean to instead test that it is a conversion from signed to unsigned
(i.e. test
if (TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (use_stmt))))
return false;
? Also, shouldn't it also test that both types are integral and have the
same precision?
> + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt))
> + return false;
> + }
> +
> if (is_gimple_assign (use_stmt)
> && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
> && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
> @@ -2099,7 +2119,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 && !integer_onep (rhs))
This doesn't look safe. orig_use_lhs in this case means
either that there was just a cast, or that there was BIT_AND_EXPR,
or that were both, and you don't know which one it is.
The decision shouldn't be done based on whether rhs is or isn't 1, but on
whether there was the BIT_AND or not.
> {
> if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
> return false;
> @@ -2345,6 +2365,8 @@ spaceship_replacement (basic_block cond_bb, basic_block
> middle_bb,
> 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 if (integer_onep (rhs))
> + res_cmp = GE_EXPR;
And this one should be guarded on either the cast present or the comparison
done unsigned (so probably TYPE_UNSIGNED (TREE_TYPE (rhs)) && integer_onep
(rhs))?
> else
> return false;
> break;
> @@ -2353,6 +2375,8 @@ spaceship_replacement (basic_block cond_bb, basic_block
> middle_bb,
> res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> else if (integer_zerop (rhs))
> res_cmp = one_cmp;
> + else if (integer_onep (rhs))
> + res_cmp = one_cmp;
> else
> return false;
> break;
Likewise.
> @@ -2360,7 +2384,7 @@ spaceship_replacement (basic_block cond_bb, basic_block
> middle_bb,
> if (integer_zerop (rhs))
> res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
> else if (integer_onep (rhs))
> - res_cmp = one_cmp;
> + res_cmp = LE_EXPR;
> else
> return false;
> break;
Are you sure?
Jakub
