This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]