Re: [PATCH PR80153]Always generate folded type conversion in tree-affine

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Thu, Mar 30, 2017 at 11:37 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Mar 29, 2017 at 5:22 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 1:34 PM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 2:01 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>> Hi,
>>>> This patch is to fix PR80153.  As analyzed in the PR, root cause is tree_affine lacks
>>>> ability differentiating (unsigned)(ptr + offset) and (unsigned)ptr + (unsigned)offset,
>>>> even worse, it always returns the former expression in aff_combination_tree, which
>>>> is wrong if the original expression has the latter form.  The patch resolves the issue
>>>> by always returning the latter form expression, i.e, always trying to generate folded
>>>> expression.  Also as analyzed in comment, I think this change won't result in substantial
>>>> code gen difference.
>>>> I also need to adjust get_computation_aff for test case gcc.dg/tree-ssa/reassoc-19.c.
>>>> Well, I think the changed behavior is correct, but for case the original pointer candidate
>>>> is chosen, it should be unnecessary to compute in uutype.  Also this adjustment only
>>>> generates (unsigned)(pointer + offset) which is generated by tree-affine.c.
>>>> Bootstrap and test on x86_64 and AArch64.  Is it OK?
>>>
>> Thanks for reviewing.
>>> Hmm.  What is the desired goal?  To have all elts added have
>>> comb->type as type?  Then
>>> the type passed to add_elt_to_tree is redundant with comb->type.  It
>>> looks like it
>>> is always passed comb->type now.
>> Yes, except pointer type comb->type, elts are converted to comb->type
>> with this patch.
>> The redundant type is removed in updated patch.
>>
>>>
>>> ISTR from past work in this area that it was important for pointer
>>> combinations to allow
>>> both pointer and sizetype elts at least.
>> Yes, It's still important to allow different types for pointer and
>> offset in pointer type comb.
>> I missed a pointer type check condition in the patch, fixed in updated patch.
>>>
>>> Your change is incomplete I think, for the scale == -1 and POINTER_TYPE_P case
>>> elt is sizetype now, not of pointer type.  As said above, we are
>>> trying to maintain
>>> both pointer and sizetype elts with like:
>>>
>>>   if (scale == 1)
>>>     {
>>>       if (!expr)
>>>         {
>>>           if (POINTER_TYPE_P (TREE_TYPE (elt)))
>>>             return elt;
>>>           else
>>>             return fold_convert (type1, elt);
>>>         }
>>>
>>> where your earilier fold to type would result in not all cases handled the same
>>> (depending whether scale was -1 for example).
>> IIUC, it doesn't matter.  For comb->type being pointer type, the
>> behavior remains the same.
>> For comb->type being unsigned T, this elt is converted to ptr_offtype,
>> rather than unsigned T,
>> this doesn't matter because ptr_offtype and unsigned T are equal to
>> each other, otherwise
>> tree_to_aff_combination shouldn't distribute it as a single elt.
>> Anyway, this is addressed in updated patch by checking pointer
>> comb->type additionally.
>> BTW, I think "scale==-1" case is a simple heuristic differentiating
>> pointer_base and offset.
>>
>>>
>>> Thus - shouldn't we simply drop the type argument (or rather the comb one?
>>> that wide_int_ext_for_comb looks weird given we get a widest_int as input
>>> and all the other wide_int_ext_for_comb calls around).
>>>
>>> And unconditionally convert to type, simplifying the rest of the code?
>> As said, for pointer type comb, we need to keep current behavior; for
>> other cases,
>> unconditionally convert to comb->type is the goal.
>>
>> Bootstrap and test on x86_64 and AArch64.  Is this version OK?
>
> @@ -399,22 +400,20 @@ add_elt_to_tree (tree expr, tree type, tree elt,
> const widest_int &scale_in,
>           if (POINTER_TYPE_P (TREE_TYPE (elt)))
>             return elt;
>           else
> -           return fold_convert (type1, elt);
> +           return fold_convert (type, elt);
>         }
>
> the conversion should already have been done.  For non-pointer comb->type
> it has been converted to type by your patch.  For pointer-type comb->type
> it should be either pointer type or ptrofftype ('type') already as well.
>
> That said, can we do sth like
>
> @@ -384,6 +395,12 @@ add_elt_to_tree (tree expr, tree type, t
>
>    widest_int scale = wide_int_ext_for_comb (scale_in, comb);
>
> +  if (! POINTER_TYPE_P (comb->type))
> +    elt = fold_convert (comb->type, elt);
> +  else
> +    gcc_assert (POINTER_TYPE_P (TREE_TYPE (elt))
> +               || types_compatible_p (TREE_TYPE (elt), type1));
Hmm, this assert can be broken since we do STRIP_NOPS converting to
aff_tree. It's not compatible for signed and unsigned integer types.
Also, with this patch, we can even support elt of short type in a
unsigned long comb, though this is useless.

> +
>    if (scale == -1
>        && POINTER_TYPE_P (TREE_TYPE (elt)))
>      {
>
> that is clearly do the conversion at the start in a way the state
> of elt is more clear?
Yes, thanks.  V3 patch attached (with gcc_assert removed).  Is it ok
after bootstrap/test?

Thanks,
bin
>
> Richard.
>
>
>
>> Thanks,
>> bin
>>
>> 2017-03-28  Bin Cheng  <bin.cheng@arm.com>
>>
>>     PR tree-optimization/80153
>>     * tree-affine.c (add_elt_to_tree): Remove parameter TYPE.  Use type
>>     of parameter COMB.  Convert elt to type of COMB it COMB is not of
>>     pointer type.
>>     (aff_combination_to_tree): Update calls to add_elt_to_tree.
>>     * tree-ssa-loop-ivopts.c (alloc_iv): Pass in consistent types.
>>     (get_computation_aff): Use utype directly for original candidate.
>>
>> gcc/testsuite/ChangeLog
>> 2017-03-28  Bin Cheng  <bin.cheng@arm.com>
>>
>>     PR tree-optimization/80153
>>     * gcc.c-torture/execute/pr80153.c: New.

Attachment: pr80153-20170330.txt
Description: Text document