This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC][PATCH][PR63586] Convert x+x+x+x into 4*x
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: kugan <kugan dot vivekanandarajah at linaro dot org>
- Cc: Christophe Lyon <christophe dot lyon at linaro dot org>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 9 May 2016 13:42:42 +0200
- Subject: Re: [RFC][PATCH][PR63586] Convert x+x+x+x into 4*x
- Authentication-results: sourceware.org; auth=none
- References: <56CFC059 dot 40108 at linaro dot org> <CAFiYyc3HM-Y526mhuuYbNSOs8pFG+Bqc_Uz30zvoU4D09c2bRA at mail dot gmail dot com> <56D3C8D9 dot 5020508 at linaro dot org> <CAKdteOYn7L=P1XZv9a9TdJ1y21zm5tk8oeSg+iJivyTiOpaXzg at mail dot gmail dot com> <CAFiYyc00ha_NSsUvjFx90RwesjV1xCpnk=K-LxNbQbeHZ5PWKQ at mail dot gmail dot com> <CAFiYyc0HGcH25jHoiGQS=TdN70swXoYh3bPyWN3orSwK5x2J6w at mail dot gmail dot com> <571BF10E dot 2060809 at linaro dot org> <CAFiYyc3=z-avkOEg0Hkas=-BRTtK9xeYdDJrf3FuwxPKKtRH-Q at mail dot gmail dot com> <572AA887 dot 7060408 at linaro dot org>
On Thu, May 5, 2016 at 3:57 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>>
>> maybe instert_stmt_after will help here, I don't think you got the
>> insertion
>> logic correct, thus insert_stmt_after (mul_stmt, def_stmt) which I think
>> misses GIMPLE_NOP handling. At least
>>
>> + if (SSA_NAME_VAR (op) != NULL
>>
>> huh? I suppose you could have tested SSA_NAME_IS_DEFAULT_DEF
>> but just the GIMPLE_NOP def-stmt test should be enough.
>>
>> + && gimple_code (def_stmt) == GIMPLE_NOP)
>> + {
>> + gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN
>> (cfun)));
>> + stmt = gsi_stmt (gsi);
>> + gsi_insert_before (&gsi, mul_stmt, GSI_NEW_STMT);
>>
>> not sure if that is the best insertion point choice, it un-does all
>> code-sinking done
>> (and no further sinking is run after the last reassoc pass). We do know
>> we
>> are handling all uses of op in our chain so inserting before the plus-expr
>> chain root should work here (thus 'stmt' in the caller context). I'd
>> use that here instead.
>> I think I'd use that unconditionally even if it works and not bother
>> finding something
>> more optimal.
>>
>
> I now tried using instert_stmt_after with special handling for GIMPLE_PHI as
> you described.
Thanks - I'd still say doing my last suggestion is likely better, at least if
'def_stmt' is not in the same basic-block as 'stmt'. So can you do
if (gimple_code (def_stmt) == GIMPLE_NOP
|| gimple_bb (stmt) != gimple_bb (def_stmt))
{
...
instead?
>
>> Apart from this this now looks ok to me.
>>
>> But the testcases need some work
>>
>>
>> --- a/gcc/testsuite/gcc.dg/tree-ssa/pr63586-2.c
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr63586-2.c
>> @@ -0,0 +1,29 @@
>> +/* { dg-do compile } */
>> ...
>> +
>> +/* { dg-final { scan-tree-dump-times "\\\*" 4 "reassoc1" } } */
>>
>> I would have expected 3.
>
>
> We now have an additional _15 = x_1(D) * 2
Ok.
> Also please check for \\\* 5 for example
>>
>> to be more specific (and change the cases so you get different constants
>> for the different functions).
>
>
>>
>> That said, please make the scans more specific.
>
>
> I have now changes the test-cases to scan more specific multiplication scan
> as you wanted.
>
>
> Does this now look better?
Yes. Ok with the above suggested change.
Thanks,
Richard.
>
> Thanks,
> Kugan