Re: [RFA] Factor conversion out of COND_EXPR using match.pd pattern

[Richard Biener <richard.guenther@gmail.com>]

On Sat, May 30, 2015 at 6:22 AM, Jeff Law <law@redhat.com> wrote:
>
> I was working on polishing some of Kai's work to eliminate shorten_compare
> and stumbled on this tiny missed optimization.
>
> Basically this change allows us to see something like this:
>
>    n = (short unsigned int) mode_size[(unsigned int) mode] * 8 <= 64 ? (int)
> ((short unsigned int) mode_size[(unsigned int) mode] * 8) : 64;
>
>
> Note the (int) cast on the true arm of the COND_EXPR.  We factor that out
> and adjust the constant (which is obviously free) into:
>
>    n = (int)((short unsigned int) mode_size[(unsigned int) mode] * 8 <= 64 ?
> ((short unsigned int) mode_size[(unsigned int) mode] * 8) : 64);
>
>
> Seems subtle, but now the existing optimizers can recognize it as:
>
> !   n = (int) MIN_EXPR <(short unsigned int) mode_size[(unsigned int) mode]
> * 8, 64>;
>
>
> In another case I saw we had the same kind of transformation occur, but
> there was already a type conversation outside the MIN_EXPR.  So we ended up
> with
>
> (T1) (T2) MIN_EXPR < ... >
>
> And we were able to prove the conversion to T2 was redundant resulting in
> just (T) MIN_EXPR < ... >
>
>
> You could legitimately ask why not apply this when both arguments are
> converted.  That leads to recursion via fold-const.c which wants to shove
> the conversion back into the arms in that case.  Removing that code results
> in testsuite regressions.  I felt it was time to cut that thread a bit as
> the real goal here is to remove the shorten_* bits in c-common, not convert
> all of fold-const.c to match.pd :-)
>
> Bootstrapped and regression tested on x86_64-linux-gnu.  OK for the trunk?
>
>
>         * match.pd: Add pattern to factor type conversion out of the
> true/false
>         arms of a COND_EXPR.
>
>         * gcc.dg/fold-cond-2.c: New test.
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index abd7851..bf4da61 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -1182,3 +1182,41 @@ along with GCC; see the file COPYING3.  If not see
>         (convert (bit_and (op (convert:utype @0) (convert:utype @1))
>                           (convert:utype @4)))))))
>
> +/* fold-const has a rich set of optimizations for expressions of the
> +   form A op B ? A : B.   However, those optimizations can be easily
> +   confused by typecasting, particularly in the true/false arms of the
> +   conditional.
> +
> +   These patterns recognize cases where the typecasting can be moved
> +   from the true/false arms to the final result.  This in turn allows
> +   fold-const to do a better job.
> +
> +   Note there is no canonical ordering for the arms of the COND_EXPR,
> +   so we have to match both variants.
> +
> +   Handling a convert in each arm runs afoul of COND_EXPR folding in
> +   fold-const.c which shoves the conversion back into each arm resulting
> +   in infinite recursion.  */
> +(simplify
> +  (cond @0 (convert @1) INTEGER_CST@2)
> +  (if (INTEGRAL_TYPE_P (TREE_TYPE (@1))
> +       && COMPARISON_CLASS_P (@0)
> +       && int_fits_type_p (@2, TREE_TYPE (@1))
> +       && ((operand_equal_p (TREE_OPERAND (@0, 0), @2, 0)
> +           && operand_equal_p (TREE_OPERAND (@0, 1), @1, 0))
> +          || (operand_equal_p (TREE_OPERAND (@0, 0), @1, 0)
> +              && operand_equal_p (TREE_OPERAND (@0, 1), @2, 0))))
> +    (with { tree itype = TREE_TYPE (@1); tree otype = TREE_TYPE (@2); }
> +      (convert:otype (cond:itype @0 @1 (convert:itype @2))))))
> +
> +(simplify
> +  (cond @0 INTEGER_CST@1 (convert @2))
> +  (if (INTEGRAL_TYPE_P (TREE_TYPE (@2))
> +       && COMPARISON_CLASS_P (@0)
> +       && int_fits_type_p (@1, TREE_TYPE (@2))
> +       && ((operand_equal_p (TREE_OPERAND (@0, 0), @2, 0)
> +           && operand_equal_p (TREE_OPERAND (@0, 1), @1, 0))
> +          || (operand_equal_p (TREE_OPERAND (@0, 0), @1, 0)
> +              && operand_equal_p (TREE_OPERAND (@0, 1), @2, 0))))
> +    (with { tree itype = TREE_TYPE (@2); tree otype = TREE_TYPE (@1); }
> +      (convert:otype (cond:itype @0 (convert:itype @1) @2)))))

Now this is a case where :c support on cond would make sense...  in
theory the preprocessor would also be your friend here.  Or we could
enhance the syntax to support multiple match patterns for the same
result, thus

 (simplify
   (cond @0 (convert @1) INTEGER_CST@2)
   (cond @0 INTEGER_CST@2 (convert @1))
   (if ...

though that would require some extra markup to see where the list of matches
ends.  user-defined predicates can be used for this already though

(match (widening_cond @0 @1 @2)
  (cond @0 (convert @1) INTEGER_CST@2))
(match (widening_cond @0 @1 @2)
  (cond @0 INTEGER_CST@2 (convert @1)))
(simplify
  (widening_cond @0 @1 @2)
  (if (...

but the comments from Marc still holds in that you shouldn't rely
on @0 being GENERIC - you want a GIMPLE testcase as well for this,
sth like

_Bool cond = 64 < mode_size[mode];
return cond ? 64 : mode_size[mode];

so to use an iterator for the comparison you'd end up with sth like

(match (widening_cond @0 @1 @2 @3)
  (cond (tcc_comparison @0 @1) (convert @2) INTEGER_CST@3))
(match (widening_cond @0 @1 @2 @3)
  (cond (tcc_comparison @0 @1) INTEGER_CST@3 (convert @2)))
(simplify
  (widening_cond @0 @1 @2 @3)
  (if ...

bah, the (match ...) syntax should allow for multiple matches without
needing repetition of (match (...) at least.

Richard.

> diff --git a/gcc/testsuite/gcc.dg/fold-cond-2.c
> b/gcc/testsuite/gcc.dg/fold-cond-2.c
> new file mode 100644
> index 0000000..2039f6e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fold-cond-2.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-original" } */
> +
> +
> +extern unsigned short mode_size[];
> +int
> +oof (int mode)
> +{
> +  return (64 < mode_size[mode] ? 64 : mode_size[mode]);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "original" } } */
> +/* { dg-final { cleanup-tree-dump "original" } } */
> +
>