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));
+
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?
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.
