RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values

[Tamar Christina <Tamar.Christina@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 6340 bytes --]

> > +      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, &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.

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 (!single_imm_use (orig_use_lhs, &use_p, &use_stmt))
+	return false;
+
+      is_cast = true;
+    }
+
   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 +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)
     {
       if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
 	return false;
@@ -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;

[-- Attachment #2: rb14938.patch --]
[-- Type: application/octet-stream, Size: 2537 bytes --]

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 (!single_imm_use (orig_use_lhs, &use_p, &use_stmt))
+	return false;
+
+      is_cast = true;
+    }
+
   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 +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)
     {
       if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
 	return false;
@@ -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;