Message ID | 54242F1B.7060909@arm.com |
---|---|
State | New |
Headers | show |
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
On 25/09/14 17:24, Jeff Law wrote: > On 09/25/14 09:04, Jiong Wang wrote: >> 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. committed as r215611. -- Jiong > > Jeff > > >
On Thu, Sep 25, 2014 at 9:43 AM, Jiong Wang <jiong.wang@arm.com> wrote: > > On 25/09/14 17:24, Jeff Law wrote: >> >> On 09/25/14 09:04, Jiong Wang wrote: >>> >>> 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. This fails on Linux/x86 and with -m32 on Linux/x86-64. >> With that addition, OK for the trunk. > > > committed as r215611.
On 26/09/14 16:05, H.J. Lu wrote: > On Thu, Sep 25, 2014 at 9:43 AM, Jiong Wang <jiong.wang@arm.com> wrote: >> On 25/09/14 17:24, Jeff Law wrote: >>> On 09/25/14 09:04, Jiong Wang wrote: >>>> 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. > This fails on Linux/x86 and with -m32 on Linux/x86-64. sorry, my test machine is x86-64, I think the shrink wrap test itself is very fragile because it's highly related insn generated. could you mark that testcase using something like "dg-require-effective-target lp64"? > >>> With that addition, OK for the trunk. >> >> committed as r215611. > >
diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c index af23f02..bd4813c 100644 --- a/gcc/shrink-wrap.c +++ b/gcc/shrink-wrap.c @@ -250,16 +250,21 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn, if (!df_live) return false; + basic_block old_dest = live_edge->dest; next_block = split_edge (live_edge); /* We create a new basic block. Call df_grow_bb_info to make sure all data structures are allocated. */ df_grow_bb_info (df_live); - bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb)); + + bitmap_and (df_get_live_in (next_block), df_get_live_out (bb), + df_get_live_in (old_dest)); df_set_bb_dirty (next_block); /* We should not split more than once for a function. */ - gcc_assert (!(*split_p)); + if (*split_p) + return false; + *split_p = true; } diff --git a/gcc/testsuite/gcc.target/i386/shrink_wrap_1.c b/gcc/testsuite/gcc.target/i386/shrink_wrap_1.c new file mode 100644 index 0000000..47f2468 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/shrink_wrap_1.c @@ -0,0 +1,48 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-rtl-pro_and_epilogue" } */ + +enum machine_mode +{ + FAKE_0, + FAKE_1, + FAKE_2, + FAKE_3, + FAKE_4, + FAKE_5, + NUM_MACHINE_MODES, +}; + +typedef int *rtx; +typedef long unsigned int size_t; +extern unsigned char mode_size[NUM_MACHINE_MODES]; + +extern rtx c_readstr (const char *, enum machine_mode); +extern rtx convert_to_mode (enum machine_mode, rtx, int); +extern rtx expand_mult (enum machine_mode, rtx, rtx, rtx, int); +extern rtx force_reg (enum machine_mode, rtx); +extern void *memset (void *__s, int __c, size_t __n); + +rtx +builtin_memset_gen_str (void *data, long offset __attribute__ ((__unused__)), + enum machine_mode mode) +{ + rtx target, coeff; + size_t size; + char *p; + + size = ((unsigned short) (__builtin_constant_p (mode) + ? mode_size_inline (mode) : mode_size[mode])); + if (size == 1) + return (rtx) data; + + p = ((char *) __builtin_alloca(sizeof (char) * (size))); + memset (p, 1, size); + coeff = c_readstr (p, mode); + + target = convert_to_mode (mode, (rtx) data, 1); + target = expand_mult (mode, target, coeff, (rtx) 0, 1); + return force_reg (mode, target); +} + +/* { dg-final { scan-rtl-dump "Performing shrink-wrapping" "pro_and_epilogue" } } */ +/* { dg-final { cleanup-rtl-dump "pro_and_epilogue" } } */