On Thu, Nov 24, 2016 at 11:11 AM, Bin.Cheng <amker.ch...@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 8:57 AM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Wed, Nov 23, 2016 at 3:57 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>> On Wed, Nov 23, 2016 at 2:27 PM, Richard Biener
>>> <richard.guent...@gmail.com> wrote:
>>>> On Wed, Nov 23, 2016 at 2:54 PM, Bin Cheng <bin.ch...@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.ch...@arm.com>
>>>>>
>>>>> * match.pd: Refine type conversion in result expressions for below
>>>>> pattern:
>>>>> (cond (cmp (convert1? x) c1) (convert2? x) c2) -> (minmax (x c)).
