diff mbox

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

Message ID 54242F1B.7060909@arm.com
State New
Headers show

Commit Message

Jiong Wang Sept. 25, 2014, 3:04 p.m. UTC 
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".

Comments

Jeff Law Sept. 25, 2014, 4:24 p.m. UTC | #1 
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
Jiong Wang Sept. 25, 2014, 4:43 p.m. UTC | #2 
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
>
>
>
H.J. Lu Sept. 26, 2014, 3:05 p.m. UTC | #3 
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.
Jiong Wang Sept. 26, 2014, 3:14 p.m. UTC | #4 
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 mbox

Patch

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" } } */