Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Fri, Nov 12, 2021 at 07:30:35AM +, 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 =
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
Ping
> -Original Message-
> From: Tamar Christina
> Sent: Friday, November 12, 2021 7:31 AM
> To: Jakub Jelinek
> Cc: Jonathan Wakely ; Richard Biener
> ; gcc-patches@gcc.gnu.org; nd
> Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> on signed values
>
>
>
> > -Original Message-
> > From: Jakub Jelinek
> > Sent: Thursday, November 4, 2021 4:11 PM
> > To: Tamar Christina
> > Cc: Jonathan Wakely ; Richard Biener
> > ; gcc-patches@gcc.gnu.org; nd
> > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear
> > patterns on signed values
> >
> > On Thu, Nov 04, 2021 at 12:19:34PM +, Tamar Christina wrote:
> > > I'm not sure the precision matters since if the conversion resulted
> > > in not enough precision such that It influences the compare it would
> > > have
> > been optimized out.
> >
> > You can't really rely on other optimizations being performed. They
> > will usually happen, but might not because such code only materialized
> > short time ago without folding happening in between, or some debug
> > counters or -
> > fno-* disabling some passes, ...
>
> Fair point, I have separated out the logic as you requested and added the
> debug fix.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> and no regressions.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
> codegen.
>
> --- inline copy of patch ---
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index
> 0e339c46afa29fa97f90d9bc4394370cd9b4b396..3ad5b23885a37eec0beff229e2
> a96e86658b2d1a 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -2038,11 +2038,36 @@ 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));
> - if (is_gimple_assign (use_stmt)
> - && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
> - && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
> - && (wi::to_wide (gimple_assign_rhs2 (use_stmt))
> - == wi::shifted_mask (1, prec - 1, false, prec)))
> + bool is_cast = false;
> +
> + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0
> + into res <= 1 and has left a type-cast for signed types. */
> + 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. */
> + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt));
> + tree ty2 = TREE_TYPE (orig_use_lhs);
> +
> + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2))
> + return false;
> + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2))
> + return false;
> + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
> + return false;
> + if (EDGE_COUNT (phi_bb->preds) != 4)
> + return false;
> + if (!single_imm_use (orig_use_lhs, _p, _stmt))
> + return false;
> +
> + is_cast = true;
> +}
> + else if (is_gimple_assign (use_stmt)
> +&& gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
> +&& TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
> +&& (wi::to_wide (gimple_assign_rhs2 (use_stmt))
> +== wi::shifted_mask (1, prec - 1, false, prec)))
> {
>/* For partial_ordering result operator>= with unspec as second
>argument is (res & 1) == res, folded by match.pd into @@ -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;
> @@ -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
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
> -Original Message-
> From: Jakub Jelinek
> Sent: Thursday, November 4, 2021 4:11 PM
> To: Tamar Christina
> Cc: Jonathan Wakely ; Richard Biener
> ; gcc-patches@gcc.gnu.org; nd
> Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> on signed values
>
> On Thu, Nov 04, 2021 at 12:19:34PM +, Tamar Christina wrote:
> > I'm not sure the precision matters since if the conversion resulted in
> > not enough precision such that It influences the compare it would have
> been optimized out.
>
> You can't really rely on other optimizations being performed. They will
> usually happen, but might not because such code only materialized short
> time ago without folding happening in between, or some debug counters or -
> fno-* disabling some passes, ...
Fair point, I have separated out the logic as you requested and added the debug
fix.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
codegen.
--- inline copy of patch ---
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index
0e339c46afa29fa97f90d9bc4394370cd9b4b396..3ad5b23885a37eec0beff229e2a96e86658b2d1a
100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -2038,11 +2038,36 @@ 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));
- if (is_gimple_assign (use_stmt)
- && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
- && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
- && (wi::to_wide (gimple_assign_rhs2 (use_stmt))
- == wi::shifted_mask (1, prec - 1, false, prec)))
+ bool is_cast = false;
+
+ /* Deal with the case when match.pd has rewritten the (res & ~1) == 0
+ into res <= 1 and has left a type-cast for signed types. */
+ 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. */
+ tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt));
+ tree ty2 = TREE_TYPE (orig_use_lhs);
+
+ if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2))
+ return false;
+ if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2))
+ return false;
+ if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
+ return false;
+ if (EDGE_COUNT (phi_bb->preds) != 4)
+ return false;
+ if (!single_imm_use (orig_use_lhs, _p, _stmt))
+ return false;
+
+ is_cast = true;
+}
+ else if (is_gimple_assign (use_stmt)
+ && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
+ && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
+ && (wi::to_wide (gimple_assign_rhs2 (use_stmt))
+ == wi::shifted_mask (1, prec - 1, false, prec)))
{
/* For partial_ordering result operator>= with unspec as second
argument is (res & 1) == res, folded by match.pd into
@@ -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;
@@ -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;
- el
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Thu, Nov 04, 2021 at 12:19:34PM +, Tamar Christina wrote:
> I'm not sure the precision matters since if the conversion resulted in not
> enough
> precision such that It influences the compare it would have been optimized
> out.
You can't really rely on other optimizations being performed. They will
usually happen, but might not because such code only materialized short time
ago without folding happening in between, or some debug counters or -fno-*
disabling some passes, ...
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -2038,6 +2038,34 @@ 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));
> + bool is_cast = false;
> +
> + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0
> + into res <= 1 and has left a type-cast for signed types. */
> + 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. */
> + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt));
> + tree ty2 = TREE_TYPE (orig_use_lhs);
gimple_assign_rhs1 (use_stmt) is I think guaranteed to be phires
here. And that has some of this checked already at the start of
the function:
if (!INTEGRAL_TYPE_P (TREE_TYPE (phires))
|| TYPE_UNSIGNED (TREE_TYPE (phires))
> +
> + if (TYPE_UNSIGNED (ty1) || !INTEGRAL_TYPE_P (ty1))
> + return false;
So I think the above two lines are redundant.
> + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2))
> + return false;
> + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2))
> + return false;
> + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
> + return false;
> + if (EDGE_COUNT (phi_bb->preds) != 4)
> + return false;
> + if (!single_imm_use (orig_use_lhs, _p, _stmt))
> + return false;
> +
> + is_cast = true;
> +}
> +
>if (is_gimple_assign (use_stmt)
I'd feel much safer if this was else if rather than if.
The reason for the patch is that (res & ~1) == 0 is optimized
into (unsigned) res <= 1, right, so it can be either this or that
and you don't need both. If you want to also handle both, that would
mean figuring all the details even for that case, handling of debug stmts
etc.
>&& gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
>&& TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
> @@ -2099,7 +2127,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)
Because otherwise it is unclear what the above means, the
intent is that the if handles the case where BIT_AND_EXPR is present,
but with both cast to unsigned and BIT_AND_EXPR present it acts differently.
> @@ -2345,6 +2373,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) && is_cast)
> + res_cmp = GE_EXPR;
>else
> return false;
>break;
> @@ -2353,6 +2383,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) && is_cast)
> + res_cmp = LE_EXPR;
>else
> return false;
>break;
I'm afraid this is still wrong. Because is_cast which implies
that the comparison is done in unsigned type rather than signed type
which is otherwise ensured changes everything the code assumes.
While maybe EQ_EXPR and NE_EXPR will work the same whether it is unsigned or
signed comparison, the other comparisons certainly will not.
So, my preference would be instead of doing these 2 hunks handle the is_cast
case early, right before if (orig_use_lhs) above. Something like:
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;
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
> > + 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?
>
I'm not sure the precision matters since if the conversion resulted in not
enough
precision such that It influences the compare it would have been optimized out.
But I've added the check nonetheless.
> > + if (!single_imm_use (orig_use_lhs, _p, _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.
Right in the original patch I guarded this based on whether the conversion
was detected or not. I removed it because I thought it was safe enough but
have added it back now.
>
> > {
> >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?
>
No, this part is wrong, was a vim yank failure I should have checked the patch
before attaching.
Here's an updated patch.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
codegen.
--- inline copy of patch ---
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index
0e339c46afa29fa97f90d9bc4394370cd9b4b396..e72677087da72c8fa52e159f434c51bdebfc5f2d
100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -2038,6 +2038,34 @@ 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));
+ bool is_cast = false;
+
+ /* Deal with the case when match.pd has rewritten the (res & ~1) == 0
+ into res <= 1 and has left a type-cast for signed types. */
+ 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. */
+ tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt));
+ tree ty2 = TREE_TYPE (orig_use_lhs);
+
+ if (TYPE_UNSIGNED (ty1) || !INTEGRAL_TYPE_P (ty1))
+ return false;
+ if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2))
+ return false;
+ if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2))
+ return false;
+ if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
+ return false;
+ if (EDGE_COUNT (phi_bb->preds) != 4)
+ return false;
+ if
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Wed, Nov 03, 2021 at 10:56:30AM +, 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, _p, _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
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
Hi,
I think it was lost along the way that I did post an update to the detection
code to fix the regression
I think I have a better understanding of the code now and have updated the
patch.
Essentially when a signed comparison is encountered my match.pd pattern will
trigger for
EQ and NE. It rewrites these by inserting an unsigned cast and does a unsigned
comparison
With a modified immediate.
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.
Given this this patch adds additional checks for when the value is an unsigned
type conversion
and the comparison value on the rhs is 1. Since the match.pd pattern rewrites
this to either LE or GT
those are the only two conditions I accept with a rhs of 1 for and then set the
appropriate resulting
comparison based on what I understand the spaceship operator to be doing.
This fixes the regression and gets the same codegen as before.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
codegen.
--- inline copy of patch ---
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index
0e339c46afa29fa97f90d9bc4394370cd9b4b396..1a2f9294e5c3e6a7fd5ade4c21cdedc44e70d911
100644
--- 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. */
+ 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. */
+ 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;
+ if (!single_imm_use (orig_use_lhs, _p, _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))
{
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;
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;
@@ -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;
rb14938.patch
Description: rb14938.patch
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021 at 20:40, Jakub Jelinek wrote: > On Tue, Oct 26, 2021 at 08:35:40PM +0100, Jonathan Wakely wrote: > > We can change __cmp_cat::type if that would result in better code. I > picked > > signed char because we only need two bits, and preferably have a signed > > type as it simplifies some things. Would int make more sense? Or int:2 ? > > I think signed char is fine (and changing it would be an ABI change), We haven't committed to a C++20 ABI yet, so changes are possible. If we don't need to change, that's obviously preferable. > int is > unnecessarily large and int:2 would be certainly slower (and harder). > OK, signed char it is then.
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, Oct 26, 2021 at 08:35:40PM +0100, Jonathan Wakely wrote: > We can change __cmp_cat::type if that would result in better code. I picked > signed char because we only need two bits, and preferably have a signed > type as it simplifies some things. Would int make more sense? Or int:2 ? I think signed char is fine (and changing it would be an ABI change), int is unnecessarily large and int:2 would be certainly slower (and harder). Jakub
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021 at 14:36, Jakub Jelinek wrote:
> On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote:
> > On Tue, 26 Oct 2021, Jakub Jelinek wrote:
> >
> > > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote:
> > > > try
> > > > auto c = ...;
> > > > signed char c2 = c;
> > > > return c2 >= ...
> > > > then
> > >
> > > That won't work, at least when using , which is what we with
> the
> > > optimization want to deal with primarily.
> > > Because std::partial_ordering etc. aren't implicitly nor explicitly
> > > convertible to int or signed char etc.
> > > Sure, one could in the testcase define its own std::strong_ordering
> etc.
> > > and define a conversion operator for it...
> >
> > So how do we end up with the signed char case in the first place?
> > Is the frontend using a type that's target dependent?
>
> uses explicitly signed char:
> namespace std
> {
> // [cmp.categories], comparison category types
> namespace __cmp_cat
> {
> using type = signed char;
> enum class _Ord : type { equivalent = 0, less = -1, greater = 1 };
> enum class _Ncmp : type { _Unordered = 2 };
> ...
> and __cmp_cat::type is what is used as type of _M_value of std::*_ordering
> -fsigned-char vs. -funsigned-char make no difference on the testcases on
> x86, but as mentioned in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24
> some target decisions like load_extend_op uses in fold-const.c can affect
> it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html
>
>
We can change __cmp_cat::type if that would result in better code. I picked
signed char because we only need two bits, and preferably have a signed
type as it simplifies some things. Would int make more sense? Or int:2 ?
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021, Jakub Jelinek wrote:
> On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote:
> > On Tue, 26 Oct 2021, Jakub Jelinek wrote:
> >
> > > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote:
> > > > try
> > > > auto c = ...;
> > > > signed char c2 = c;
> > > > return c2 >= ...
> > > > then
> > >
> > > That won't work, at least when using , which is what we with the
> > > optimization want to deal with primarily.
> > > Because std::partial_ordering etc. aren't implicitly nor explicitly
> > > convertible to int or signed char etc.
> > > Sure, one could in the testcase define its own std::strong_ordering etc.
> > > and define a conversion operator for it...
> >
> > So how do we end up with the signed char case in the first place?
> > Is the frontend using a type that's target dependent?
>
> uses explicitly signed char:
> namespace std
> {
> // [cmp.categories], comparison category types
> namespace __cmp_cat
> {
> using type = signed char;
> enum class _Ord : type { equivalent = 0, less = -1, greater = 1 };
> enum class _Ncmp : type { _Unordered = 2 };
> ...
> and __cmp_cat::type is what is used as type of _M_value of std::*_ordering
> -fsigned-char vs. -funsigned-char make no difference on the testcases on
> x86, but as mentioned in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24
> some target decisions like load_extend_op uses in fold-const.c can affect
> it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html
Eh, I see. So there are alrady testcases covering the issues on
the affected targets.
So ignore my comment about adding additional testcases.
Richard.
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote:
> On Tue, 26 Oct 2021, Jakub Jelinek wrote:
>
> > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote:
> > > try
> > > auto c = ...;
> > > signed char c2 = c;
> > > return c2 >= ...
> > > then
> >
> > That won't work, at least when using , which is what we with the
> > optimization want to deal with primarily.
> > Because std::partial_ordering etc. aren't implicitly nor explicitly
> > convertible to int or signed char etc.
> > Sure, one could in the testcase define its own std::strong_ordering etc.
> > and define a conversion operator for it...
>
> So how do we end up with the signed char case in the first place?
> Is the frontend using a type that's target dependent?
uses explicitly signed char:
namespace std
{
// [cmp.categories], comparison category types
namespace __cmp_cat
{
using type = signed char;
enum class _Ord : type { equivalent = 0, less = -1, greater = 1 };
enum class _Ncmp : type { _Unordered = 2 };
...
and __cmp_cat::type is what is used as type of _M_value of std::*_ordering
-fsigned-char vs. -funsigned-char make no difference on the testcases on
x86, but as mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24
some target decisions like load_extend_op uses in fold-const.c can affect
it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html
Jakub
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021, Jakub Jelinek wrote: > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > > try > > auto c = ...; > > signed char c2 = c; > > return c2 >= ... > > then > > That won't work, at least when using , which is what we with the > optimization want to deal with primarily. > Because std::partial_ordering etc. aren't implicitly nor explicitly > convertible to int or signed char etc. > Sure, one could in the testcase define its own std::strong_ordering etc. > and define a conversion operator for it... So how do we end up with the signed char case in the first place? Is the frontend using a type that's target dependent? Richard.
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > try > auto c = ...; > signed char c2 = c; > return c2 >= ... > then That won't work, at least when using , which is what we with the optimization want to deal with primarily. Because std::partial_ordering etc. aren't implicitly nor explicitly convertible to int or signed char etc. Sure, one could in the testcase define its own std::strong_ordering etc. and define a conversion operator for it... Jakub
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021, Tamar Christina wrote:
>
>
> > -Original Message-
> > From: Richard Biener
> > Sent: Tuesday, October 26, 2021 9:46 AM
> > To: Tamar Christina
> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> >
> > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> > on signed values
> >
> > On Tue, 26 Oct 2021, Tamar Christina wrote:
> >
> > > > -Original Message-
> > > > From: Richard Biener
> > > > Sent: Tuesday, October 26, 2021 9:26 AM
> > > > To: Tamar Christina
> > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> > > >
> > > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear
> > > > patterns on signed values
> > > >
> > > > On Mon, 25 Oct 2021, Tamar Christina wrote:
> > > >
> > > > > > -Original Message-----
> > > > > > From: Richard Biener
> > > > > > Sent: Friday, October 15, 2021 12:31 PM
> > > > > > To: Tamar Christina
> > > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ;
> > > > > > nd
> > > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with
> > > > > > bitclear patterns on signed values
> > > > > >
> > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote:
> > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > > During testing after rebasing to commit I noticed a failing
> > > > > > > testcase with the bitmask compare patch.
> > > > > > >
> > > > > > > Consider the following C++ testcase:
> > > > > > >
> > > > > > > #include
> > > > > > >
> > > > > > > #define A __attribute__((noipa)) A bool f5 (double i, double
> > > > > > > j) { auto c = i <=> j; return c >= 0; }
> > > > > > >
> > > > > > > This turns into a comparison against chars, on systems where
> > > > > > > chars are signed the pattern inserts an unsigned convert such
> > > > > > > that it's able to do the transformation.
> > > > > > >
> > > > > > > i.e.:
> > > > > > >
> > > > > > > # RANGE [-1, 2]
> > > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > > > # RANGE ~[3, 254]
> > > > > > > _11 = (unsigned char) c$_M_value_22;
> > > > > > > _19 = _11 <= 1;
> > > > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > > > D.10434 ={v} {CLOBBER};
> > > > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > > > D.10407 ={v} {CLOBBER};
> > > > > > > # VUSE <.MEM_14>
> > > > > > > return _19;
> > > > > > >
> > > > > > > instead of:
> > > > > > >
> > > > > > > # RANGE [-1, 2]
> > > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > > > # RANGE [-2, 2]
> > > > > > > _3 = c$_M_value_5 & -2;
> > > > > > > _19 = _3 == 0;
> > > > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > > > D.10440 ={v} {CLOBBER};
> > > > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > > > D.10413 ={v} {CLOBBER};
> > > > > > > # VUSE <.MEM_14>
> > > > > > > return _19;
> > > > > > >
> > > > > > > This causes much worse codegen under -ffast-math due to phiops
> > > > > > > no longer recognizing the pattern. It turns out that phiopts
> > > > > > > spaceship_replacement is looking for the exact form that was
> > > > > > > just
> > > > changed.
> > > > > > >
> > > > > > > Trying to get it to recognize the new form is not trivial as
> > > > > > > the transformation doesn't look to work when the thing it's
> > > > > > > pointing to is itself
> > > > > > a phi-node.
> > > > > >
> > > > > > What do you mean? Where it handles t
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
> -Original Message-
> From: Richard Biener
> Sent: Tuesday, October 26, 2021 9:46 AM
> To: Tamar Christina
> Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
>
> Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> on signed values
>
> On Tue, 26 Oct 2021, Tamar Christina wrote:
>
> > > -Original Message-
> > > From: Richard Biener
> > > Sent: Tuesday, October 26, 2021 9:26 AM
> > > To: Tamar Christina
> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> > >
> > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear
> > > patterns on signed values
> > >
> > > On Mon, 25 Oct 2021, Tamar Christina wrote:
> > >
> > > > > -Original Message-
> > > > > From: Richard Biener
> > > > > Sent: Friday, October 15, 2021 12:31 PM
> > > > > To: Tamar Christina
> > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ;
> > > > > nd
> > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with
> > > > > bitclear patterns on signed values
> > > > >
> > > > > On Fri, 15 Oct 2021, Tamar Christina wrote:
> > > > >
> > > > > > Hi All,
> > > > > >
> > > > > > During testing after rebasing to commit I noticed a failing
> > > > > > testcase with the bitmask compare patch.
> > > > > >
> > > > > > Consider the following C++ testcase:
> > > > > >
> > > > > > #include
> > > > > >
> > > > > > #define A __attribute__((noipa)) A bool f5 (double i, double
> > > > > > j) { auto c = i <=> j; return c >= 0; }
> > > > > >
> > > > > > This turns into a comparison against chars, on systems where
> > > > > > chars are signed the pattern inserts an unsigned convert such
> > > > > > that it's able to do the transformation.
> > > > > >
> > > > > > i.e.:
> > > > > >
> > > > > > # RANGE [-1, 2]
> > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > > # RANGE ~[3, 254]
> > > > > > _11 = (unsigned char) c$_M_value_22;
> > > > > > _19 = _11 <= 1;
> > > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > > D.10434 ={v} {CLOBBER};
> > > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > > D.10407 ={v} {CLOBBER};
> > > > > > # VUSE <.MEM_14>
> > > > > > return _19;
> > > > > >
> > > > > > instead of:
> > > > > >
> > > > > > # RANGE [-1, 2]
> > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > > # RANGE [-2, 2]
> > > > > > _3 = c$_M_value_5 & -2;
> > > > > > _19 = _3 == 0;
> > > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > > D.10440 ={v} {CLOBBER};
> > > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > > D.10413 ={v} {CLOBBER};
> > > > > > # VUSE <.MEM_14>
> > > > > > return _19;
> > > > > >
> > > > > > This causes much worse codegen under -ffast-math due to phiops
> > > > > > no longer recognizing the pattern. It turns out that phiopts
> > > > > > spaceship_replacement is looking for the exact form that was
> > > > > > just
> > > changed.
> > > > > >
> > > > > > Trying to get it to recognize the new form is not trivial as
> > > > > > the transformation doesn't look to work when the thing it's
> > > > > > pointing to is itself
> > > > > a phi-node.
> > > > >
> > > > > What do you mean? Where it handles the BIT_AND it could also
> > > > > handle the conversion, no? The later handling would probably
> > > > > more explicitely need to distinguish between the BIT_AND and the
> > > > > conversion
> > > forms.
> > > >
> > > > Looks like I misunderstood the code, it was looking at the uses
> > > > not the defs of the value.
> > > >
> > > > --- inline copy of patch ---
> > > >
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Tue, 26 Oct 2021, Tamar Christina wrote:
> > -Original Message-
> > From: Richard Biener
> > Sent: Tuesday, October 26, 2021 9:26 AM
> > To: Tamar Christina
> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> >
> > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> > on signed values
> >
> > On Mon, 25 Oct 2021, Tamar Christina wrote:
> >
> > > > -Original Message-
> > > > From: Richard Biener
> > > > Sent: Friday, October 15, 2021 12:31 PM
> > > > To: Tamar Christina
> > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> > > >
> > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear
> > > > patterns on signed values
> > > >
> > > > On Fri, 15 Oct 2021, Tamar Christina wrote:
> > > >
> > > > > Hi All,
> > > > >
> > > > > During testing after rebasing to commit I noticed a failing
> > > > > testcase with the bitmask compare patch.
> > > > >
> > > > > Consider the following C++ testcase:
> > > > >
> > > > > #include
> > > > >
> > > > > #define A __attribute__((noipa))
> > > > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0;
> > > > > }
> > > > >
> > > > > This turns into a comparison against chars, on systems where chars
> > > > > are signed the pattern inserts an unsigned convert such that it's
> > > > > able to do the transformation.
> > > > >
> > > > > i.e.:
> > > > >
> > > > > # RANGE [-1, 2]
> > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > # RANGE ~[3, 254]
> > > > > _11 = (unsigned char) c$_M_value_22;
> > > > > _19 = _11 <= 1;
> > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > D.10434 ={v} {CLOBBER};
> > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > D.10407 ={v} {CLOBBER};
> > > > > # VUSE <.MEM_14>
> > > > > return _19;
> > > > >
> > > > > instead of:
> > > > >
> > > > > # RANGE [-1, 2]
> > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > > # RANGE [-2, 2]
> > > > > _3 = c$_M_value_5 & -2;
> > > > > _19 = _3 == 0;
> > > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > > D.10440 ={v} {CLOBBER};
> > > > > # .MEM_14 = VDEF <.MEM_24>
> > > > > D.10413 ={v} {CLOBBER};
> > > > > # VUSE <.MEM_14>
> > > > > return _19;
> > > > >
> > > > > This causes much worse codegen under -ffast-math due to phiops no
> > > > > longer recognizing the pattern. It turns out that phiopts
> > > > > spaceship_replacement is looking for the exact form that was just
> > changed.
> > > > >
> > > > > Trying to get it to recognize the new form is not trivial as the
> > > > > transformation doesn't look to work when the thing it's pointing
> > > > > to is itself
> > > > a phi-node.
> > > >
> > > > What do you mean? Where it handles the BIT_AND it could also handle
> > > > the conversion, no? The later handling would probably more
> > > > explicitely need to distinguish between the BIT_AND and the conversion
> > forms.
> > >
> > > Looks like I misunderstood the code, it was looking at the uses not
> > > the defs of the value.
> > >
> > > --- inline copy of patch ---
> > >
> > > The comments seems to suggest this code only checks for (res & ~1) ==
> > > 0 but the implementation seems to suggest it's broader.
> > >
> > > As such I added a case to check to see if the value comparison we
> > > found is a type cast. and strips away the type cast and continues.
> > >
> > > In match.pd the typecasts are only added for signed comparisons to ==
> > > 0 and != 0 which are then rewritten into comparisons with 1.
> > >
> > > As such I only check for 1 and LE and GT, which is what match.pd would
> > > have rewritten it to.
> > >
> > > This fixes the regression but this is not code I 100% understand,
> >
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
> -Original Message-
> From: Richard Biener
> Sent: Tuesday, October 26, 2021 9:26 AM
> To: Tamar Christina
> Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
>
> Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> on signed values
>
> On Mon, 25 Oct 2021, Tamar Christina wrote:
>
> > > -Original Message-
> > > From: Richard Biener
> > > Sent: Friday, October 15, 2021 12:31 PM
> > > To: Tamar Christina
> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> > >
> > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear
> > > patterns on signed values
> > >
> > > On Fri, 15 Oct 2021, Tamar Christina wrote:
> > >
> > > > Hi All,
> > > >
> > > > During testing after rebasing to commit I noticed a failing
> > > > testcase with the bitmask compare patch.
> > > >
> > > > Consider the following C++ testcase:
> > > >
> > > > #include
> > > >
> > > > #define A __attribute__((noipa))
> > > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0;
> > > > }
> > > >
> > > > This turns into a comparison against chars, on systems where chars
> > > > are signed the pattern inserts an unsigned convert such that it's
> > > > able to do the transformation.
> > > >
> > > > i.e.:
> > > >
> > > > # RANGE [-1, 2]
> > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > # RANGE ~[3, 254]
> > > > _11 = (unsigned char) c$_M_value_22;
> > > > _19 = _11 <= 1;
> > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > D.10434 ={v} {CLOBBER};
> > > > # .MEM_14 = VDEF <.MEM_24>
> > > > D.10407 ={v} {CLOBBER};
> > > > # VUSE <.MEM_14>
> > > > return _19;
> > > >
> > > > instead of:
> > > >
> > > > # RANGE [-1, 2]
> > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > > # RANGE [-2, 2]
> > > > _3 = c$_M_value_5 & -2;
> > > > _19 = _3 == 0;
> > > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > > D.10440 ={v} {CLOBBER};
> > > > # .MEM_14 = VDEF <.MEM_24>
> > > > D.10413 ={v} {CLOBBER};
> > > > # VUSE <.MEM_14>
> > > > return _19;
> > > >
> > > > This causes much worse codegen under -ffast-math due to phiops no
> > > > longer recognizing the pattern. It turns out that phiopts
> > > > spaceship_replacement is looking for the exact form that was just
> changed.
> > > >
> > > > Trying to get it to recognize the new form is not trivial as the
> > > > transformation doesn't look to work when the thing it's pointing
> > > > to is itself
> > > a phi-node.
> > >
> > > What do you mean? Where it handles the BIT_AND it could also handle
> > > the conversion, no? The later handling would probably more
> > > explicitely need to distinguish between the BIT_AND and the conversion
> forms.
> >
> > Looks like I misunderstood the code, it was looking at the uses not
> > the defs of the value.
> >
> > --- inline copy of patch ---
> >
> > The comments seems to suggest this code only checks for (res & ~1) ==
> > 0 but the implementation seems to suggest it's broader.
> >
> > As such I added a case to check to see if the value comparison we
> > found is a type cast. and strips away the type cast and continues.
> >
> > In match.pd the typecasts are only added for signed comparisons to ==
> > 0 and != 0 which are then rewritten into comparisons with 1.
> >
> > As such I only check for 1 and LE and GT, which is what match.pd would
> > have rewritten it to.
> >
> > This fixes the regression but this is not code I 100% understand,
> > since I don't really know the semantics of the spaceship operator so
> > would appreciate an extra look.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> > and no regressions.
> >
> > Ok for master?
>
> Please add a testcase. I hope Jakub can review the spaceship_replacement
> patch since he's the one familiar with the code.
There's already a bunch of testcases that test the various variants:
gcc/testsuite/g++.dg/opt/pr94589-1.C
and gcc/testsuite/g++.dg/opt
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Mon, 25 Oct 2021, Tamar Christina wrote:
> > -Original Message-
> > From: Richard Biener
> > Sent: Friday, October 15, 2021 12:31 PM
> > To: Tamar Christina
> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
> >
> > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> > on signed values
> >
> > On Fri, 15 Oct 2021, Tamar Christina wrote:
> >
> > > Hi All,
> > >
> > > During testing after rebasing to commit I noticed a failing testcase
> > > with the bitmask compare patch.
> > >
> > > Consider the following C++ testcase:
> > >
> > > #include
> > >
> > > #define A __attribute__((noipa))
> > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; }
> > >
> > > This turns into a comparison against chars, on systems where chars are
> > > signed the pattern inserts an unsigned convert such that it's able to
> > > do the transformation.
> > >
> > > i.e.:
> > >
> > > # RANGE [-1, 2]
> > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > # RANGE ~[3, 254]
> > > _11 = (unsigned char) c$_M_value_22;
> > > _19 = _11 <= 1;
> > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > D.10434 ={v} {CLOBBER};
> > > # .MEM_14 = VDEF <.MEM_24>
> > > D.10407 ={v} {CLOBBER};
> > > # VUSE <.MEM_14>
> > > return _19;
> > >
> > > instead of:
> > >
> > > # RANGE [-1, 2]
> > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > > # RANGE [-2, 2]
> > > _3 = c$_M_value_5 & -2;
> > > _19 = _3 == 0;
> > > # .MEM_24 = VDEF <.MEM_6(D)>
> > > D.10440 ={v} {CLOBBER};
> > > # .MEM_14 = VDEF <.MEM_24>
> > > D.10413 ={v} {CLOBBER};
> > > # VUSE <.MEM_14>
> > > return _19;
> > >
> > > This causes much worse codegen under -ffast-math due to phiops no
> > > longer recognizing the pattern. It turns out that phiopts
> > > spaceship_replacement is looking for the exact form that was just changed.
> > >
> > > Trying to get it to recognize the new form is not trivial as the
> > > transformation doesn't look to work when the thing it's pointing to is
> > > itself
> > a phi-node.
> >
> > What do you mean? Where it handles the BIT_AND it could also handle the
> > conversion, no? The later handling would probably more explicitely need to
> > distinguish between the BIT_AND and the conversion forms.
>
> Looks like I misunderstood the code, it was looking at the uses not the defs
> of
> the value.
>
> --- inline copy of patch ---
>
> The comments seems to suggest this code only checks for (res & ~1) == 0 but
> the
> implementation seems to suggest it's broader.
>
> As such I added a case to check to see if the value comparison we found is a
> type cast. and strips away the type cast and continues.
>
> In match.pd the typecasts are only added for signed comparisons to == 0 and
> != 0
> which are then rewritten into comparisons with 1.
>
> As such I only check for 1 and LE and GT, which is what match.pd would have
> rewritten it to.
>
> This fixes the regression but this is not code I 100% understand, since I
> don't
> really know the semantics of the spaceship operator so would appreciate an
> extra
> look.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu,
> x86_64-pc-linux-gnu and no regressions.
>
> Ok for master?
Please add a testcase. I hope Jakub can review the spaceship_replacement
patch since he's the one familiar with the code.
Thanks,
Richard.
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
> codegen.
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index
> 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78aa2340418571a33
> 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, basic_block
> middle_bb,
>tree lhs, rhs;
>gimple *orig_use_stmt = use_stmt;
>tree orig_use_lhs = NULL_TREE;
> + bool is_canon = false;
>int prec = TYPE_PRECISION (TREE_TYPE (phires));
>if (is_gimple_assign (use_stmt)
>&& gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
> @@ -2063,6 +2064,26 @@ spaceship_replacement (basic_block cond_bb,
> basic_block middle_
RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
> -Original Message-
> From: Richard Biener
> Sent: Friday, October 15, 2021 12:31 PM
> To: Tamar Christina
> Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd
>
> Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> on signed values
>
> On Fri, 15 Oct 2021, Tamar Christina wrote:
>
> > Hi All,
> >
> > During testing after rebasing to commit I noticed a failing testcase
> > with the bitmask compare patch.
> >
> > Consider the following C++ testcase:
> >
> > #include
> >
> > #define A __attribute__((noipa))
> > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; }
> >
> > This turns into a comparison against chars, on systems where chars are
> > signed the pattern inserts an unsigned convert such that it's able to
> > do the transformation.
> >
> > i.e.:
> >
> > # RANGE [-1, 2]
> > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > # RANGE ~[3, 254]
> > _11 = (unsigned char) c$_M_value_22;
> > _19 = _11 <= 1;
> > # .MEM_24 = VDEF <.MEM_6(D)>
> > D.10434 ={v} {CLOBBER};
> > # .MEM_14 = VDEF <.MEM_24>
> > D.10407 ={v} {CLOBBER};
> > # VUSE <.MEM_14>
> > return _19;
> >
> > instead of:
> >
> > # RANGE [-1, 2]
> > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > # RANGE [-2, 2]
> > _3 = c$_M_value_5 & -2;
> > _19 = _3 == 0;
> > # .MEM_24 = VDEF <.MEM_6(D)>
> > D.10440 ={v} {CLOBBER};
> > # .MEM_14 = VDEF <.MEM_24>
> > D.10413 ={v} {CLOBBER};
> > # VUSE <.MEM_14>
> > return _19;
> >
> > This causes much worse codegen under -ffast-math due to phiops no
> > longer recognizing the pattern. It turns out that phiopts
> > spaceship_replacement is looking for the exact form that was just changed.
> >
> > Trying to get it to recognize the new form is not trivial as the
> > transformation doesn't look to work when the thing it's pointing to is
> > itself
> a phi-node.
>
> What do you mean? Where it handles the BIT_AND it could also handle the
> conversion, no? The later handling would probably more explicitely need to
> distinguish between the BIT_AND and the conversion forms.
Looks like I misunderstood the code, it was looking at the uses not the defs of
the value.
--- inline copy of patch ---
The comments seems to suggest this code only checks for (res & ~1) == 0 but the
implementation seems to suggest it's broader.
As such I added a case to check to see if the value comparison we found is a
type cast. and strips away the type cast and continues.
In match.pd the typecasts are only added for signed comparisons to == 0 and != 0
which are then rewritten into comparisons with 1.
As such I only check for 1 and LE and GT, which is what match.pd would have
rewritten it to.
This fixes the regression but this is not code I 100% understand, since I don't
really know the semantics of the spaceship operator so would appreciate an extra
look.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
codegen.
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index
0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78aa2340418571a33
100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, basic_block
middle_bb,
tree lhs, rhs;
gimple *orig_use_stmt = use_stmt;
tree orig_use_lhs = NULL_TREE;
+ bool is_canon = false;
int prec = TYPE_PRECISION (TREE_TYPE (phires));
if (is_gimple_assign (use_stmt)
&& gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
@@ -2063,6 +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block
middle_bb,
}
else if (is_gimple_assign (use_stmt))
{
+ /* Deal with if match.pd has rewritten the (res & ~1) == 0
+into res <= 1 and has left a type-cast for signed types. */
+ if (gimple_assign_cast_p (use_stmt))
+ {
+ orig_use_lhs = gimple_assign_lhs (use_stmt);
+ 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;
+ if (!single_imm_use (orig_use_lhs, _p, _stmt))
+ return false;
+ tree_code cmp;
+ if (is_gimple_assign (use_stmt)
+ && (cmp = gimple_assign_rhs_code (use_stmt))
+ && (cmp ==
Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
On Fri, 15 Oct 2021, Tamar Christina wrote:
> Hi All,
>
> During testing after rebasing to commit I noticed a failing testcase with the
> bitmask compare patch.
>
> Consider the following C++ testcase:
>
> #include
>
> #define A __attribute__((noipa))
> A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; }
>
> This turns into a comparison against chars, on systems where chars are signed
> the pattern inserts an unsigned convert such that it's able to do the
> transformation.
>
> i.e.:
>
> # RANGE [-1, 2]
> # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> # RANGE ~[3, 254]
> _11 = (unsigned char) c$_M_value_22;
> _19 = _11 <= 1;
> # .MEM_24 = VDEF <.MEM_6(D)>
> D.10434 ={v} {CLOBBER};
> # .MEM_14 = VDEF <.MEM_24>
> D.10407 ={v} {CLOBBER};
> # VUSE <.MEM_14>
> return _19;
>
> instead of:
>
> # RANGE [-1, 2]
> # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> # RANGE [-2, 2]
> _3 = c$_M_value_5 & -2;
> _19 = _3 == 0;
> # .MEM_24 = VDEF <.MEM_6(D)>
> D.10440 ={v} {CLOBBER};
> # .MEM_14 = VDEF <.MEM_24>
> D.10413 ={v} {CLOBBER};
> # VUSE <.MEM_14>
> return _19;
>
> This causes much worse codegen under -ffast-math due to phiops no longer
> recognizing the pattern. It turns out that phiopts spaceship_replacement is
> looking for the exact form that was just changed.
>
> Trying to get it to recognize the new form is not trivial as the
> transformation
> doesn't look to work when the thing it's pointing to is itself a phi-node.
What do you mean? Where it handles the BIT_AND it could also handle
the conversion, no? The later handling would probably more explicitely
need to distinguish between the BIT_AND and the conversion forms.
Jakub?
> Because of these issues this change delays the replacements until after loop
> opts. This fixes the failing C++ testcase.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu,
> x86_64-pc-linux-gnu and no regressions.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> * match.pd: Delay bitmask compare pattern till after loop opts.
>
> --- inline copy of patch --
> diff --git a/gcc/match.pd b/gcc/match.pd
> index
> 9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03
> 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -4945,7 +4945,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>icmp (le le gt le gt)
> (simplify
>(cmp (bit_and:c@2 @0 cst@1) integer_zerop)
> - (with { tree csts = bitmask_inv_cst_vector_p (@1); }
> + (if (canonicalize_math_after_vectorization_p ())
> +(with { tree csts = bitmask_inv_cst_vector_p (@1); }
> (switch
>(if (csts && TYPE_UNSIGNED (TREE_TYPE (@1))
> && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
> @@ -4954,7 +4955,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> && (cmp == EQ_EXPR || cmp == NE_EXPR)
> && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
> (with { tree utype = unsigned_type_for (TREE_TYPE (@1)); }
> - (icmp (convert:utype @0) { csts; }
> + (icmp (convert:utype @0) { csts; })
>
> /* -A CMP -B -> B CMP A. */
> (for cmp (tcc_comparison)
>
>
>
--
Richard Biener
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
[PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values
Hi All,
During testing after rebasing to commit I noticed a failing testcase with the
bitmask compare patch.
Consider the following C++ testcase:
#include
#define A __attribute__((noipa))
A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; }
This turns into a comparison against chars, on systems where chars are signed
the pattern inserts an unsigned convert such that it's able to do the
transformation.
i.e.:
# RANGE [-1, 2]
# c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
# RANGE ~[3, 254]
_11 = (unsigned char) c$_M_value_22;
_19 = _11 <= 1;
# .MEM_24 = VDEF <.MEM_6(D)>
D.10434 ={v} {CLOBBER};
# .MEM_14 = VDEF <.MEM_24>
D.10407 ={v} {CLOBBER};
# VUSE <.MEM_14>
return _19;
instead of:
# RANGE [-1, 2]
# c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
# RANGE [-2, 2]
_3 = c$_M_value_5 & -2;
_19 = _3 == 0;
# .MEM_24 = VDEF <.MEM_6(D)>
D.10440 ={v} {CLOBBER};
# .MEM_14 = VDEF <.MEM_24>
D.10413 ={v} {CLOBBER};
# VUSE <.MEM_14>
return _19;
This causes much worse codegen under -ffast-math due to phiops no longer
recognizing the pattern. It turns out that phiopts spaceship_replacement is
looking for the exact form that was just changed.
Trying to get it to recognize the new form is not trivial as the transformation
doesn't look to work when the thing it's pointing to is itself a phi-node.
Because of these issues this change delays the replacements until after loop
opts. This fixes the failing C++ testcase.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* match.pd: Delay bitmask compare pattern till after loop opts.
--- inline copy of patch --
diff --git a/gcc/match.pd b/gcc/match.pd
index
9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03
100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4945,7 +4945,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
icmp (le le gt le gt)
(simplify
(cmp (bit_and:c@2 @0 cst@1) integer_zerop)
- (with { tree csts = bitmask_inv_cst_vector_p (@1); }
+ (if (canonicalize_math_after_vectorization_p ())
+(with { tree csts = bitmask_inv_cst_vector_p (@1); }
(switch
(if (csts && TYPE_UNSIGNED (TREE_TYPE (@1))
&& (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
@@ -4954,7 +4955,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& (cmp == EQ_EXPR || cmp == NE_EXPR)
&& (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
(with { tree utype = unsigned_type_for (TREE_TYPE (@1)); }
- (icmp (convert:utype @0) { csts; }
+ (icmp (convert:utype @0) { csts; })
/* -A CMP -B -> B CMP A. */
(for cmp (tcc_comparison)
--
diff --git a/gcc/match.pd b/gcc/match.pd
index 9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4945,7 +4945,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
icmp (le le gt le gt)
(simplify
(cmp (bit_and:c@2 @0 cst@1) integer_zerop)
- (with { tree csts = bitmask_inv_cst_vector_p (@1); }
+ (if (canonicalize_math_after_vectorization_p ())
+(with { tree csts = bitmask_inv_cst_vector_p (@1); }
(switch
(if (csts && TYPE_UNSIGNED (TREE_TYPE (@1))
&& (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
@@ -4954,7 +4955,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& (cmp == EQ_EXPR || cmp == NE_EXPR)
&& (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2)))
(with { tree utype = unsigned_type_for (TREE_TYPE (@1)); }
- (icmp (convert:utype @0) { csts; }
+ (icmp (convert:utype @0) { csts; })
/* -A CMP -B -> B CMP A. */
(for cmp (tcc_comparison)
