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

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

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?

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-20170328.txt
Description: Text document