[PATCH, 2/2] shrink wrap a function with a single loop: split live_edge

[Jeff Law]

On 09/25/14 09:04, Jiong Wang wrote:
>
> On 25/09/14 09:52, Zhenqiang Chen wrote:
>>
>>> -----Original Message-----
>>> From: Jiong Wang [mailto:jiong.wang@arm.com]
>>> Sent: Thursday, September 25, 2014 2:13 AM
>>> To: Jeff Law; Zhenqiang Chen
>>> Cc: gcc-patches@gcc.gnu.org
>>> Subject: Re: [PATCH, 2/2] shrink wrap a function with a single loop:
>>> split
>>> live_edge
>>>
>>>
>>> On 22/09/14 18:51, Jeff Law wrote:
>>>> On 09/22/14 04:24, Jiong Wang wrote:
>>>>>> Great.  Can you send an updated patchkit for review.
>>>>> patch attached.
>>>>>
>>>>> please review, thanks.
>>>>>
>>>>> gcc/ * shrink-wrap.c (move_insn_for_shrink_wrap): Initialize the
>>>>> live-in of new created BB as the intersection of live-in from
>>>>> "old_dest" and live-out from "bb".
>>>> Looks good.  However, before committing we need a couple things.
>>>>
>>>> 1. Bootstrap & regression test this variant of the patch.  I know you
>>>> tested an earlier one, but please test this one just to be sure.
>>>>
>>>> 2. Testcase.  I think you could test for either the reduction in the
>>>> live-in set of the newly created block or that you're shrink wrapping
>>>> one or more functions you didn't previously shrink-wrap.  I think it's
>>>> fine if this test is target specific.
>>>    bootstrap ok based on revision 215515.
>>>
>>>    while the x86 regression result is interesting. there is no
>>> regression on
>>> check-g++, while there is four regression on check-gcc:
>>>
>>> FAIL: gcc.dg/tree-ssa/loadpre10.c (internal compiler error)
>>> FAIL: gcc.dg/tree-ssa/loadpre10.c (test for excess errors)
>>> FAIL: gcc.dg/tree-ssa/pr21417.c (internal compiler error)
>>> FAIL: gcc.dg/tree-ssa/pr21417.c (test for excess errors)
>>>
>>>     this is caused by our improving the accuracy of live-in for new
>>> created basic
>>> block. Now we will split
>>>     more than one edge for the above two testcase. thus trigger the
>>> following
>>> assert in move_insn_for_shrink_wrap:
>>>
>>>         /* We should not split more than once for a function.  */
>>>         gcc_assert (!(*split_p));
>> According to the algorithm, it is impossible to split one edge twice.
>> It's possible to split two different edges. But for such cases, the
>> control flow is too complex to perform shrink-wrapping.
>>
>> Anyway, your patch improves the accuracy. You can replace the
>> "gcc_assert" to "return"; or change "split_p" to "splitted_edge" then
>> you can check one edge is not splitted twice.
>
> thanks for the explanation.
>
> actually, the old "bitmap_copy (df_get_live_in (next_block),
> df_get_live_out (bb));" will let any "dest" reg
> in entry block alive in the new splitted block. If there is another
> block which "dest" also set in live_in, then
> dest alive in two blocks, then those code in "live_edge_for_reg" will
> always return NULL, thus the old
> inaccurate data flow will actually never make split two different edges
> happen... thus assert never triggered.
>
> as from the whole x86 boostrap, and regression test, only two cases
> trigger split two different edges, I think it's
> trival case, thus prefer to be conservative to keep the old logic, as
> suggested, just replace "gcc_assert" into "return false".
>
> or if we want to allow multi split, I think just remove the assert is
> OK, because "EDGE_COUNT (next_block->preds) == 2"
> will guarantee split one edge twice never happen.
>
> new patch updated.
>
> pass bootstrap and no regression, both check-gcc and check-g++, on the x86.
>
> OK for trunk?
>
> thanks.
>
> gcc/
>     * shrink-wrap.c (move_insn_for_shrink_wrap): Initialize the live-in of
>     new created BB as the intersection of live-in from "old_dest" and
> live-out
>     from "bb".
Please include a ChangeLog entry for the testsuite.  Something like:

	* gcc.target/i386/shrink_wrap_1.c: New test.

With that addition, OK for the trunk.

Jeff