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

[Jiong Wang <jiong dot wang at arm dot com>]

On 19/09/14 06:51, Jeff Law wrote:
> On 09/16/14 00:55, Zhenqiang Chen wrote:
> > > -----Original Message-----
> > > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > > owner@gcc.gnu.org] On Behalf Of Jiong Wang
> > > Sent: Monday, September 15, 2014 11:28 PM
> > > To: Zhenqiang Chen
> > > Cc: gcc-patches@gcc.gnu.org; Jeff Law
> > > Subject: Re: [PATCH, 2/2] shrink wrap a function with a single loop: split
> > > live_edge
> > >
> > > On 08/05/14 09:07, Zhenqiang Chen wrote:
> > > >     static bool
> > > >     move_insn_for_shrink_wrap (basic_block bb, rtx insn,
> > > >                               const HARD_REG_SET uses,
> > > >                               const HARD_REG_SET defs,
> > > > -                          HARD_REG_SET *last_uses)
> > > > +                          HARD_REG_SET *last_uses,
> > > > +                          bool *split_p)
> > > > {
> > > >       rtx set, src, dest;
> > > >       bitmap live_out, live_in, bb_uses, bb_defs;
> > > >       unsigned int i, dregno, end_dregno, sregno, end_sregno;
> > > >       basic_block next_block;
> > > > +  edge live_edge;
> > > >
> > > >       /* Look for a simple register copy.  */
> > > >       set = single_set (insn);
> > > > @@ -5582,17 +5589,31 @@ move_insn_for_shrink_wrap (basic_block bb,
> > > rtx insn,
> > > >           || overlaps_hard_reg_set_p (defs, GET_MODE (dest), dregno))
> > > >         return false;
> > > >
> > > > -  /* See whether there is a successor block to which we could move
> > > > INSN.  */
> > > > -  next_block = next_block_for_reg (bb, dregno, end_dregno);
> > > > -  if (!next_block)
> > > > +  live_edge = next_block_for_reg (bb, dregno, end_dregno);  if
> > > > + (!live_edge)
> > > >          return false;
> > > >
> > > > +  next_block = live_edge->dest;
> > > > +
> > > >       /* If the destination register is referred in later insn,
> > > >          try to forward it.  */
> > > >       if (overlaps_hard_reg_set_p (*last_uses, GET_MODE (dest), dregno)
> > > >           && !try_copy_prop (bb, insn, src, dest, last_uses))
> > > >         return false;
> > > >
> > > > +  /* Create a new basic block on the edge.  */  if (EDGE_COUNT
> > > > + (next_block->preds) == 2)
> > > > +    {
> > > > +      next_block = split_edge (live_edge);
> > > > +
> > > > +      bitmap_copy (df_get_live_in (next_block), df_get_live_out
> > > > + (bb));
> > > (re-sent, looks like the first send not received by the server...)
> > >
> > >     for the function "_IO_wdefault_xsputn" in glibc wgenops.c (.dot file
> > > attached)
> > >
> > >     174: NOTE_INSN_BASIC_BLOCK 21 are the new created basic block because
> > > of the sink of move instruction "ax:DI = 0" and split edge.
> > >
> > >     but the live_in of this BB is copied from live_out of BB 2 which is too big, and
> > > actually prevent the later sink of "16: r12:DI=dx:DI".
> > >
> > >     Should it be better to copy live_in from "next_block->next_bb" instead of
> > > live_out from "bb", as it will model what's needed more accurately?
> > According to the algorithm, next_block->next_bb (which is live_edge->dest) should have two predecessors. Live_in of next_block->next_bb would include live_out of the other edge,  which is not necessary accurately. To be more accurate, you may need an intersection of df_get_live_out (bb) and df_get_live_in (next_block->next_bb).
> Presumably we can't recompute the liveness info here as it's too
> expensive to do that each time we split an edge to sink a statement?
> ie, we need the more accurate liveness within this pass so that we can
> sink further instructions?

for a single case, x86 bootstrap, looks like these code are not compile time critical
as for all 4762 live edges which could sink instructions

4139: EDGE_COUNT (next_block->preds) == 1
270: EDGE_COUNT (next_block->preds) == 2
353: EDGE_COUNT (next_block->preds) > 2

and if we make the live info more accurate here, just very few additional functions (< 10)
shrink-wrapped from glibc build & gcc bootstrap test.

So, is it OK we simply change "bitmap_copy  (df_get_live_in  (next_block),  df_get_live_out  (bb));"
into "bitmap_and  (df_get_live_in  (next_block),  df_get_live_out  (bb),  df_get_live_in  (next_block->next_bb));" ?

-- Jiong

> jeff