Re: [PATCH GCC]Refine type conversion in result expressions for cond_expr pattern

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Nov 24, 2016 at 11:11 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>>> Hi,
>>>>> This is actually the review suggestion for patch @https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02341.html, but I forgot to incorporate it when committing that patch.  Here comes this one doing that, as well as adding a missing convert keyword.  Toolchain built successfully, is it OK?
>>>>
>>>> As said you _do_ need the outermost (convert ...) on the (max .. and
>>>> (min ... expressions given @1 may not be of type 'type'.
>>> Sorry about the stupid mistake.  How about this one?  The from_type in
>>> the last branch looks like necessary to me.
>>
>> I think
>>
>>      (if (code == EQ_EXPR)
>>       (cond (cmp @1 (convert @3)) (convert @3) @2)))))))
>>
>> is better?  We want the outer expression of type 'type' and @2 is
>> already 'type',
>> only @3 may not be.  So the only change would be to dop the unnecessary
>> :from_type inside the cmp and the bogus :from_type on the true arg of the cond.
> Hi Richard,
> The idea of using from_type in EQ_EXPR case is to do cond_expr in
> narrow/from type for all its operands, then convert the result back to
> default type.

I see.

> -      (cond (cmp @1 (convert:from_type @3)) (convert:from_type @3) @2)))))))
> +      (convert (cond (cmp @1 (convert @3))
> +     (convert:from_type @3) (convert:from_type @2)))))))))
>
> Is it better than using different types for operand 0 and 1/2 in cond_expr?

Ah, that's a valid point...

> I updated the patch following your suggestion.  Note, in this way
> below range check on @2 should be redundant for EQ_EXPR case, but I
> didn't change that in this patch.
>
>      if (int_fits_type_p (@2, from_type)
>      && (types_match (c1_type, from_type)
>          || (TYPE_PRECISION (c1_type) > TYPE_PRECISION (from_type)
>          && (TYPE_UNSIGNED (from_type)
>              || TYPE_SIGN (c1_type) == TYPE_SIGN (from_type))))
>
> So which one should be preferred?

I suppose it's better to use the same type and thus your version then
(-20161123).
That patch is ok.

Note my worry here is usually that we already have conflicting foldings in this
area (moving conversions in/out), see fold_unary:

          /* If this was a conversion, and all we did was to move into
             inside the COND_EXPR, bring it back out.  But leave it if
             it is a conversion from integer to integer and the
             result precision is no wider than a word since such a
             conversion is cheap and may be optimized away by combine,
             while it couldn't if it were outside the COND_EXPR.  Then return
             so we don't get into an infinite recursion loop taking the
             conversion out and then back in.  */

          if ((CONVERT_EXPR_CODE_P (code)
               || code == NON_LVALUE_EXPR)
              && TREE_CODE (tem) == COND_EXPR
              && TREE_CODE (TREE_OPERAND (tem, 1)) == code
              && TREE_CODE (TREE_OPERAND (tem, 2)) == code
              && ! VOID_TYPE_P (TREE_OPERAND (tem, 1))
              && ! VOID_TYPE_P (TREE_OPERAND (tem, 2))
              && (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0))
                  == TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 2), 0)))
              && (! (INTEGRAL_TYPE_P (TREE_TYPE (tem))
                     && (INTEGRAL_TYPE_P
                         (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (tem, 1), 0))))
                     && TYPE_PRECISION (TREE_TYPE (tem)) <= BITS_PER_WORD)
                  || flag_syntax_only))
            tem = build1_loc (loc, code, type,
                              build3 (COND_EXPR,
                                      TREE_TYPE (TREE_OPERAND
                                                 (TREE_OPERAND (tem, 1), 0)),
                                      TREE_OPERAND (tem, 0),
                                      TREE_OPERAND (TREE_OPERAND (tem, 1), 0),
                                      TREE_OPERAND (TREE_OPERAND (tem, 2),
                                                    0)));

and fold_ternary has quite a bit of COND_EXPR folding as well.

Thanks,
Richard.

>
> Thanks,
> bin
>>
>> Richard.
>>
>>
>>> Thanks,
>>> bin
>>>>
>>>>> Thanks,
>>>>> bin
>>>>>
>>>>> 2016-11-23  Bin Cheng  <bin.cheng@arm.com>
>>>>>
>>>>>         * match.pd: Refine type conversion in result expressions for below
>>>>>         pattern:
>>>>>         (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).