This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH GCC]Refine type conversion in result expressions for cond_expr pattern
On Thu, Nov 24, 2016 at 11:17 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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)));
Yes, this is a potential issue. I will apply this version at the
moment, we can always fall
back to the other way if the conflict mentioned below becomes a real
problem, especially
for EQ_EXPR case.
>
> and fold_ternary has quite a bit of COND_EXPR folding as well.
The new pattern is moved from fold_cond_expr_with_comparison which in
turn is called
from fold_ternary. There isn't much conflict between it and the rest
cases in fold_ternary.
IIUC, they target at different simplifications.
I can add comment about conflict with fold_unary in the pattern, but
would like to do that
after fixing ICEs reported in PR78507 and PR78510.
Thanks,
bin
>
> 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)).