On Thu, Mar 30, 2017 at 3:20 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
> 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);
Yeah, that looks good to me.
>>
>> 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.
