On Thu, Mar 30, 2017 at 2:18 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
> On Thu, Mar 30, 2017 at 1:44 PM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Thu, Mar 30, 2017 at 2:03 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>> On Thu, Mar 30, 2017 at 11:37 AM, Richard Biener
>>> <richard.guent...@gmail.com> wrote:
>>>> On Wed, Mar 29, 2017 at 5:22 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>>>> On Tue, Mar 28, 2017 at 1:34 PM, Richard Biener
>>>>> <richard.guent...@gmail.com> wrote:
>>>>>> On Tue, Mar 28, 2017 at 2:01 PM, Bin Cheng <bin.ch...@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?
>>
>> - return fold_build2 (PLUS_EXPR, type1,
>> - expr, fold_convert (type1, elt));
>> + return fold_build2 (PLUS_EXPR, type, expr, fold_convert (type, elt));
>>
>> folding not needed(?)
>>
>> - return fold_build1 (NEGATE_EXPR, type1,
>> - fold_convert (type1, elt));
>> + return fold_build1 (NEGATE_EXPR, type, fold_convert (type, elt));
>>
>> likewise.
>>
>> - return fold_build2 (MINUS_EXPR, type1,
>> - expr, fold_convert (type1, elt));
>> + return fold_build2 (MINUS_EXPR, type, expr, fold_convert (type, elt));
>>
>> likewise.
>>
>> Ok with removing those and re-testing.
> Hmm, I thought twice about the simplification, there are cases not
> properly handled:
>>>> + 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));
> This is not enough, for pointer type comb, if elt is the offset part,
> we could return signed integer type elt without folding. Though this
> shouldn't be an issue because it's always converted to ptr_offtype in
> building pointer_plus, it's better not to create such expressions in
> the first place. Check condition for unconditionally converting elt
> should be improved as:
>>>> + if (! POINTER_TYPE_P (comb->type) || !POINTER_TYPE_P (TREE_TYPE (elt)))
>>>> + elt = fold_convert (comb->type, elt);
Hmm, precisely as:
>>>> + if (! POINTER_TYPE_P (comb->type) || !POINTER_TYPE_P (TREE_TYPE (elt)))
>>>> + elt = fold_convert (type, elt);
>
> With this change, folds can be removed as you suggested. I will test
> new patch for this.
>
> Thanks,
> bin
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> bin
>>>>
>>>> Richard.
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> bin
>>>>>
>>>>> 2017-03-28 Bin Cheng <bin.ch...@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.ch...@arm.com>
>>>>>
>>>>> PR tree-optimization/80153
>>>>> * gcc.c-torture/execute/pr80153.c: New.
