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

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Nov 04, 2021 at 12:19:34PM +0000, 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, &use_p, &use_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;
	  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)
    {
...
Similarly to the BIT_AND_EXPR case the above transforms
the is_cast case into a signed comparison that the later code
knows how to handle.

There is another problem though.  If there are debug stmts, the
code later on does:
          /* If there are debug uses, emit something like:
             # DEBUG D#1 => i_2(D) > j_3(D) ? 1 : -1
             # DEBUG D#2 => i_2(D) == j_3(D) ? 0 : D#1
             where > stands for the comparison that yielded 1
             and replace debug uses of phi result with that D#2.
             Ignore the value of 2, because if NaNs aren't expected,
             all floating point numbers should be comparable.  */
...
          replace_uses_by (phires, temp2);
          if (orig_use_lhs)
            replace_uses_by (orig_use_lhs, temp2);
For the is_cast case this creates invalid IL, because the uses of
orig_use_lhs if any expect an unsigned value, but temp2 is signed.
Perhaps when one uses <compare> stuff this will never happen, but
there is nothing that prevents a user to write his own code so that
spaceship_replacement actually triggers on it (i.e. results in the
same IL) and has debug info uses on it.  And we can't e.g. punt if
there are debug stmts and not otherwise because that would result
in -fcompare-debug differences.
So, I think we need:
       bool has_debug_uses = false;
+      bool has_cast_debug_uses = false;
...
-          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;
              }
and later if (has_cast_debug_uses) create another debug temporary
temp3 with TREE_TYPE set to TREE_TYPE (orig_use_lhs),
the expression (that_uns_type) temp2 and use that temp3 instead of
temp2 in replace_uses_by (orig_use_lhs, temp2);

	Jakub