Patchwork Propagate profile counts during switch expansion

login
register
mail settings
Submitter Jan Hubicka
Date Oct. 14, 2012, 3:09 p.m.
Message ID <20121014150907.GA15581@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/191365/
State New
Headers show

Comments

Jan Hubicka - Oct. 14, 2012, 3:09 p.m.
Hi,

+static inline void
+reset_out_edges_aux (basic_block bb)
+{
+  edge e;
+  edge_iterator ei;
+  FOR_EACH_EDGE(e, ei, bb->succs)
+    e->aux = (void *)0;
+}
+static inline void
+compute_cases_per_edge (gimple stmt)
+{
+  basic_block bb = gimple_bb (stmt);
+  reset_out_edges_aux (bb);
+  int ncases = gimple_switch_num_labels (stmt);
+  for (int i = ncases - 1; i >= 1; --i)
+    {
+      tree elt = gimple_switch_label (stmt, i);
+      tree lab = CASE_LABEL (elt);
+      basic_block case_bb = label_to_block_fn (cfun, lab);
+      edge case_edge = find_edge (bb, case_bb);
+      case_edge->aux = (void *)((long)(case_edge->aux) + 1);
+    }
+}

Comments and newlines per coding standard.

With the these changes, the patch is OK

Thanks,
Honza
Easwaran Raman - Oct. 15, 2012, 6:47 p.m.
On Sun, Oct 14, 2012 at 8:09 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
>
> Index: optabs.c
> ===================================================================
> --- optabs.c    (revision 191879)
> +++ optabs.c    (working copy)
> @@ -4249,7 +4249,7 @@ prepare_operand (enum insn_code icode, rtx x, int
>     we can do the branch.  */
>
>  static void
> -emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label)
> +emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label, int prob)
>  {
>    enum machine_mode optab_mode;
>    enum mode_class mclass;
> @@ -4261,7 +4261,16 @@ static void
>
>    gcc_assert (icode != CODE_FOR_nothing);
>    gcc_assert (insn_operand_matches (icode, 0, test));
> -  emit_jump_insn (GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));
> +  rtx insn = emit_insn (
> +      GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));
>
> I think we did not change to style of mixing declaration and code yet.  So
> please put declaration ahead.
Ok.

>
> I think you want to keep emit_jump_insn.  Also do nothing when profile_status
> == PROFILE_ABSENT.

Why should this be dependent on profile_status? The PROB passed could
also come from static prediction right.

> Index: cfgbuild.c
> ===================================================================
> --- cfgbuild.c  (revision 191879)
> +++ cfgbuild.c  (working copy)
> @@ -559,8 +559,11 @@ compute_outgoing_frequencies (basic_block b)
>           f->count = b->count - e->count;
>           return;
>         }
> +      else
> +        {
> +          guess_outgoing_edge_probabilities (b);
> +        }
>
> Add comment here that we rely on multiway BBs having sane probabilities already.
> You still want to do guessing when the edges out are EH. Those also can be many.
I think this should work:

-  if (single_succ_p (b))
+  else if (single_succ_p (b))
     {
       e = single_succ_edge (b);
       e->probability = REG_BR_PROB_BASE;
       e->count = b->count;
       return;
     }
-  guess_outgoing_edge_probabilities (b);
+  else
+    {
+      /* We rely on BBs with more than two successors to have sane
probabilities
+         and do not guess them here. For BBs terminated by switch statements
+         expanded to jump-table jump, we have done the right thing during
+         expansion. For EH edges, we still guess the probabilities here.  */
+      bool complex_edge = false;
+      FOR_EACH_EDGE (e, ei, b->succs)
+        if (e->flags & EDGE_COMPLEX)
+          {
+            complex_edge = true;
+            break;
+          }
+      if (complex_edge)
+        guess_outgoing_edge_probabilities (b);
+    }
+


> Index: expr.h
> ===================================================================
> --- expr.h      (revision 191879)
> +++ expr.h      (working copy)
> @@ -190,7 +190,7 @@ extern int have_sub2_insn (rtx, rtx);
>  /* Emit a pair of rtl insns to compare two rtx's and to jump
>     to a label if the comparison is true.  */
>  extern void emit_cmp_and_jump_insns (rtx, rtx, enum rtx_code, rtx,
> -                                    enum machine_mode, int, rtx);
> +                                    enum machine_mode, int, rtx, int prob=-1);
>
> Hmm, probably first appreance of this C++ construct. I suppose it is OK.
http://gcc.gnu.org/codingconventions.html#Default says it is ok for POD values.

>
> +static inline void
> +reset_out_edges_aux (basic_block bb)
> +{
> +  edge e;
> +  edge_iterator ei;
> +  FOR_EACH_EDGE(e, ei, bb->succs)
> +    e->aux = (void *)0;
> +}
> +static inline void
> +compute_cases_per_edge (gimple stmt)
> +{
> +  basic_block bb = gimple_bb (stmt);
> +  reset_out_edges_aux (bb);
> +  int ncases = gimple_switch_num_labels (stmt);
> +  for (int i = ncases - 1; i >= 1; --i)
> +    {
> +      tree elt = gimple_switch_label (stmt, i);
> +      tree lab = CASE_LABEL (elt);
> +      basic_block case_bb = label_to_block_fn (cfun, lab);
> +      edge case_edge = find_edge (bb, case_bb);
> +      case_edge->aux = (void *)((long)(case_edge->aux) + 1);
> +    }
> +}
>
> Comments and newlines per coding standard.
Ok.

> With the these changes, the patch is OK

Thanks,
Easwaran
>
> Thanks,
> Honza
Jan Hubicka - Oct. 15, 2012, 11:44 p.m.
> On Sun, Oct 14, 2012 at 8:09 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> >
> > Index: optabs.c
> > ===================================================================
> > --- optabs.c    (revision 191879)
> > +++ optabs.c    (working copy)
> > @@ -4249,7 +4249,7 @@ prepare_operand (enum insn_code icode, rtx x, int
> >     we can do the branch.  */
> >
> >  static void
> > -emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label)
> > +emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label, int prob)
> >  {
> >    enum machine_mode optab_mode;
> >    enum mode_class mclass;
> > @@ -4261,7 +4261,16 @@ static void
> >
> >    gcc_assert (icode != CODE_FOR_nothing);
> >    gcc_assert (insn_operand_matches (icode, 0, test));
> > -  emit_jump_insn (GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));
> > +  rtx insn = emit_insn (
> > +      GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));
> >
> > I think we did not change to style of mixing declaration and code yet.  So
> > please put declaration ahead.
> Ok.
> 
> >
> > I think you want to keep emit_jump_insn.  Also do nothing when profile_status
> > == PROFILE_ABSENT.
> 
> Why should this be dependent on profile_status? The PROB passed could
> also come from static prediction right.

In that case profile_status is PROFILE_GUESSED.
> I think this should work:
> 
> -  if (single_succ_p (b))
> +  else if (single_succ_p (b))
>      {
>        e = single_succ_edge (b);
>        e->probability = REG_BR_PROB_BASE;
>        e->count = b->count;
>        return;
>      }
> -  guess_outgoing_edge_probabilities (b);
> +  else
> +    {
> +      /* We rely on BBs with more than two successors to have sane
> probabilities
> +         and do not guess them here. For BBs terminated by switch statements
> +         expanded to jump-table jump, we have done the right thing during
> +         expansion. For EH edges, we still guess the probabilities here.  */
> +      bool complex_edge = false;
> +      FOR_EACH_EDGE (e, ei, b->succs)
> +        if (e->flags & EDGE_COMPLEX)
> +          {
> +            complex_edge = true;
> +            break;
> +          }
> +      if (complex_edge)
> +        guess_outgoing_edge_probabilities (b);
> +    }
> +

 OK.

Honza

Patch

Index: optabs.c
===================================================================
--- optabs.c	(revision 191879)
+++ optabs.c	(working copy)
@@ -4249,7 +4249,7 @@  prepare_operand (enum insn_code icode, rtx x, int
    we can do the branch.  */
 
 static void
-emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label)
+emit_cmp_and_jump_insn_1 (rtx test, enum machine_mode mode, rtx label, int prob)
 {
   enum machine_mode optab_mode;
   enum mode_class mclass;
@@ -4261,7 +4261,16 @@  static void
 
   gcc_assert (icode != CODE_FOR_nothing);
   gcc_assert (insn_operand_matches (icode, 0, test));
-  emit_jump_insn (GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));
+  rtx insn = emit_insn (
+      GEN_FCN (icode) (test, XEXP (test, 0), XEXP (test, 1), label));

I think we did not change to style of mixing declaration and code yet.  So
please put declaration ahead.

I think you want to keep emit_jump_insn.  Also do nothing when profile_status
== PROFILE_ABSENT.

Index: cfgbuild.c
===================================================================
--- cfgbuild.c	(revision 191879)
+++ cfgbuild.c	(working copy)
@@ -559,8 +559,11 @@  compute_outgoing_frequencies (basic_block b)
 	  f->count = b->count - e->count;
 	  return;
 	}
+      else
+        {
+          guess_outgoing_edge_probabilities (b);
+        }

Add comment here that we rely on multiway BBs having sane probabilities already.
You still want to do guessing when the edges out are EH. Those also can be many.
Index: expr.h
===================================================================
--- expr.h	(revision 191879)
+++ expr.h	(working copy)
@@ -190,7 +190,7 @@  extern int have_sub2_insn (rtx, rtx);
 /* Emit a pair of rtl insns to compare two rtx's and to jump
    to a label if the comparison is true.  */
 extern void emit_cmp_and_jump_insns (rtx, rtx, enum rtx_code, rtx,
-				     enum machine_mode, int, rtx);
+				     enum machine_mode, int, rtx, int prob=-1);

Hmm, probably first appreance of this C++ construct. I suppose it is OK.