This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH PR80153]Always generate folded type conversion in tree-affine
On Thu, Mar 30, 2017 at 1:44 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Mar 30, 2017 at 2:03 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> 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?
>
> - 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);
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.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.