From: Richard Biener <richard.guenther@gmail.com>
To: "Bin.Cheng" <amker.cheng@gmail.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH PR80153]Always generate folded type conversion in tree-affine
Date: Thu, 30 Mar 2017 13:00:00 -0000 [thread overview]
Message-ID: <CAFiYyc0+-eyAju9cirfKmasJ77TjimtvaP-BkO34GmhoTJ_SBA@mail.gmail.com> (raw)
In-Reply-To: <CAHFci2-aE8M9s7NVGLA1rrEtsQ0JMpiy+wv_BWBxfUQ00G0Dsw@mail.gmail.com>
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.
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.
next prev parent reply other threads:[~2017-03-30 12:44 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-28 12:17 Bin Cheng
2017-03-28 12:39 ` Richard Biener
2017-03-29 15:32 ` Bin.Cheng
2017-03-30 10:46 ` Richard Biener
2017-03-30 12:44 ` Bin.Cheng
2017-03-30 13:00 ` Richard Biener [this message]
2017-03-30 13:20 ` Bin.Cheng
2017-03-30 13:34 ` Bin.Cheng
2017-03-30 14:37 ` Richard Biener
2017-04-05 7:25 ` Bin.Cheng
2017-04-05 7:26 ` Bin.Cheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc0+-eyAju9cirfKmasJ77TjimtvaP-BkO34GmhoTJ_SBA@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=amker.cheng@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).