Selective scheduler fixes

Selective scheduler fixes

[Alexander Monakov]

Hello,

This is a series of selective scheduler bug fixes.  They fix problems that
have been discovered during internal testing, and one patch is necessary as
preparation to predication support in the selective scheduler (predication
patches will be submitted separately later).

I'm sorry that patches come without testcases, but as most of the problems
have been discovered on a patched scheduler, it's virtually impossible to
provide testcases that would fail without a patch and pass with it.

The patches have been bootstrapped and regtested together on x86-64 and ia64
(without java on ia64).  Additionally, a number of tests was done with arm
cross-compiler.  I'll commit approved patches separately.

OK for trunk?

Dmitry Melnik (3):
  Take maximum spec when merging exprs
  Make more insns unique
  Drop an incorrect assert

Sergey Grechanik (5):
  Fix usage of hard_regno_nregs before reload
  Properly loop over all hard regs for mode
  Try successors to find seqno
  Factor out caching logic for INSN_COND
  Only merge deps status for true dependencies

 gcc/sched-deps.c   |   69 +++++++++++++++++++-------------
 gcc/sched-int.h    |    2 +-
 gcc/sel-sched-ir.c |  111 +++++++++++++++++++++++++++++++++++++++++-----------
 gcc/sel-sched-ir.h |    1 +
 gcc/sel-sched.c    |   14 +++---
 5 files changed, 137 insertions(+), 60 deletions(-)

[PATCH 1/8] Take maximum spec when merging exprs

[Alexander Monakov]

From: Dmitry Melnik <dm@ispras.ru>

EXPR_SPEC is an indicator of the speculativeness of an expression (an
instruction or just an rhs), as it is incremented each time the expression is
moved up across a conditional branch.  When merging expr attributes for
similar exprs available from two destinations of a branch, sel-sched assigns
the minimum of EXPR_SPEC's to the result, effectively making the resulting
expr non-speculative if only one of those exprs was non-speculative.  However,
since we are relying on EXPR_SPEC being a correct indication of expr's
speculativeness when deciding whether it would need a bookkeeping copy, we
really want to avoid that.

The patch changes minimum to maximum, making the code match what was
originally intended.

2011-08-04  Dmitry Melnik  <dm@ispras.ru>

	* sel-sched-ir.c (merge_expr_data): Take maximum spec.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index de7629a..5a287d0 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1810,9 +1810,9 @@ update_speculative_bits (expr_t to, expr_t from, insn_t split_point)
 void
 merge_expr_data (expr_t to, expr_t from, insn_t split_point)
 {
-  /* For now, we just set the spec of resulting expr to be minimum of the specs
-     of merged exprs.  */
-  if (EXPR_SPEC (to) > EXPR_SPEC (from))
+  /* Choose the maximum of the specs of merged exprs.  This is required
+     for correctness of bookkeeping.  */
+  if (EXPR_SPEC (to) < EXPR_SPEC (from))
     EXPR_SPEC (to) = EXPR_SPEC (from);
 
   if (split_point)

[PATCH 6/8] Try successors to find seqno

[Alexander Monakov]

From: Sergey Grechanik <mouseentity@condor.intra.ispras.ru>

This patch fixes a problem when new jumps created in
sel_redirect_edge_and_branch_force could not get correct seqnos.

get_seqno_of_a_pred is renamed to get_seqno_for_a_jump.
Implementation-wise, it supports looking at multiple predecessors, and, if
that fails, also at successors.  get_seqno_by_succs is implemented similar to
get_seqno_by_preds.

2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

	* sel-sched-ir.c (get_seqno_of_a_pred): Rename to
	get_seqno_for_a_jump.  Update the caller.
	(get_seqno_by_succs): New.  Use it ...
	(get_seqno_for_a_jump): ... here to find a seqno if looking at
	predecessors was not sufficient.
	(get_seqno_by_preds): Include head in iteration range, exclude insn.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index dd8b3da..91f9dd9 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -3940,9 +3940,39 @@ sel_luid_for_non_insn (rtx x)
   return -1;
 }
 
-/* Return seqno of the only predecessor of INSN.  */
+/*  Find the proper seqno for inserting at INSN by successors.
+    Return -1 if no successors with positive seqno exist.  */
 static int
-get_seqno_of_a_pred (insn_t insn)
+get_seqno_by_succs (rtx insn)
+{
+  basic_block bb = BLOCK_FOR_INSN (insn);
+  rtx tmp = insn, end = BB_END (bb);
+  int seqno;
+  insn_t succ = NULL;
+  succ_iterator si;
+
+  while (tmp != end)
+    {
+      tmp = NEXT_INSN (tmp);
+      if (INSN_P (tmp))
+        return INSN_SEQNO (tmp);
+    }
+
+  seqno = INT_MAX;
+
+  FOR_EACH_SUCC_1 (succ, si, end, SUCCS_NORMAL)
+    if (INSN_SEQNO (succ) > 0)
+      seqno = MIN (seqno, INSN_SEQNO (succ));
+
+  if (seqno == INT_MAX)
+    return -1;
+
+  return seqno;
+}
+
+/* Compute seqno for INSN by its preds or succs.  */
+static int
+get_seqno_for_a_jump (insn_t insn)
 {
   int seqno;
 
@@ -3982,14 +4012,24 @@ get_seqno_of_a_pred (insn_t insn)
 	  int n;
 
 	  cfg_preds (BLOCK_FOR_INSN (insn), &preds, &n);
-	  gcc_assert (n == 1);
 
-	  seqno = INSN_SEQNO (preds[0]);
+	  gcc_assert (n > 0);
+	  /* For one predecessor, use simple method.  */
+	  if (n == 1)
+	    seqno = INSN_SEQNO (preds[0]);
+	  else
+	    seqno = get_seqno_by_preds (insn);
 
 	  free (preds);
 	}
     }
 
+  /* We were unable to find a good seqno among preds.  */
+  if (seqno < 0)
+    seqno = get_seqno_by_succs (insn);
+
+  gcc_assert (seqno >= 0);
+
   return seqno;
 }
 
@@ -4004,10 +4044,11 @@ get_seqno_by_preds (rtx insn)
   int n, i, seqno;
 
   while (tmp != head)
-    if (INSN_P (tmp))
-      return INSN_SEQNO (tmp);
-    else
+    {
       tmp = PREV_INSN (tmp);
+      if (INSN_P (tmp))
+        return INSN_SEQNO (tmp);
+    }
 
   cfg_preds (bb, &preds, &n);
   for (i = 0, seqno = -1; i < n; i++)
@@ -4179,7 +4220,7 @@ init_simplejump_data (insn_t insn)
   init_expr (INSN_EXPR (insn), vinsn_create (insn, false), 0,
 	     REG_BR_PROB_BASE, 0, 0, 0, 0, 0, 0, NULL, true, false, false,
 	     false, true);
-  INSN_SEQNO (insn) = get_seqno_of_a_pred (insn);
+  INSN_SEQNO (insn) = get_seqno_for_a_jump (insn);
   init_first_time_insn_data (insn);
 }
 

[PATCH 2/8] Make more insns unique

[Alexander Monakov]

From: Dmitry Melnik <dm@ispras.ru>

This patch prevents duplicating (as bookkeeping code) instructions that are
either volatile or recognized by cannot_copy_insn_p target hook (in addition
to already present restrictions).  This avoids generating incorrect assembler
with duplicate labels on ARM.

2011-08-04  Dmitry Melnik  <dm@ispras.ru>

	* sel-sched-ir.c (init_global_and_expr_for_insn): Forbid copying of
	recognized by cannot_copy_insn_p hook and volatile instructions.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 5a287d0..8f5cd3f 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -2956,7 +2956,10 @@ init_global_and_expr_for_insn (insn_t insn)
           /* Exception handling insns are always unique.  */
           || (cfun->can_throw_non_call_exceptions && can_throw_internal (insn))
           /* TRAP_IF though have an INSN code is control_flow_insn_p ().  */
-          || control_flow_insn_p (insn))
+          || control_flow_insn_p (insn)
+          || volatile_insn_p (PATTERN (insn))
+          || (targetm.cannot_copy_insn_p
+              && targetm.cannot_copy_insn_p (insn)))
         force_unique_p = true;
       else
         force_unique_p = false;

[PATCH 7/8] Factor out caching logic for INSN_COND

[Alexander Monakov]

From: Sergey Grechanik <mouseentity@condor.intra.ispras.ru>

Sometimes we need to be able to call sched_get_condition_with_rev from
selective scheduler for an instruction with zero luid (i.e. before h_d_i_d
had been extended).  On such occasion, we need to bypass the caching and use
"old", uncached lookup.

The patch factors out caching logic to a separate function and amends it to
skip cached lookup for instructions with zero luid.  It also renames INSN_COND
to INSN_CACHED_COND to avoid clash with the same macro in predication patch
for sel-sched.

2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

	* sched-deps.c (sched_get_condition_with_rev): Rename to ...
	(sched_get_condition_with_rev_uncached): ... this.  Factor out
	condition caching logic into ...
	(sched_get_condition_with_rev): ... this.  Reimplement.  Do not
	attempt to use cache for instructions with zero luid.
	(sched_analyze_insn): Use INSN_CACHED_COND instead of INSN_COND.
	* sched-int.h (INSN_COND): Rename to INSN_CACHED_COND.

diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 343d03c..a82df5d 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -488,27 +488,13 @@ deps_may_trap_p (const_rtx mem)
 
 /* Find the condition under which INSN is executed.  If REV is not NULL,
    it is set to TRUE when the returned comparison should be reversed
-   to get the actual condition.
-   We only do actual work the first time we come here for an insn; the
-   results are cached in INSN_COND and INSN_REVERSE_COND.  */
+   to get the actual condition.  */
 static rtx
-sched_get_condition_with_rev (const_rtx insn, bool *rev)
+sched_get_condition_with_rev_uncached (const_rtx insn, bool *rev)
 {
   rtx pat = PATTERN (insn);
   rtx src;
 
-  if (INSN_COND (insn) == const_true_rtx)
-    return NULL_RTX;
-
-  if (INSN_COND (insn) != NULL_RTX)
-    {
-      if (rev)
-	*rev = INSN_REVERSE_COND (insn);
-      return INSN_COND (insn);
-    }
-
-  INSN_COND (insn) = const_true_rtx;
-  INSN_REVERSE_COND (insn) = false;
   if (pat == 0)
     return 0;
 
@@ -516,10 +502,7 @@ sched_get_condition_with_rev (const_rtx insn, bool *rev)
     *rev = false;
 
   if (GET_CODE (pat) == COND_EXEC)
-    {
-      INSN_COND (insn) = COND_EXEC_TEST (pat);
-      return COND_EXEC_TEST (pat);
-    }
+    return COND_EXEC_TEST (pat);
 
   if (!any_condjump_p (insn) || !onlyjump_p (insn))
     return 0;
@@ -527,10 +510,7 @@ sched_get_condition_with_rev (const_rtx insn, bool *rev)
   src = SET_SRC (pc_set (insn));
 
   if (XEXP (src, 2) == pc_rtx)
-    {
-      INSN_COND (insn) = XEXP (src, 0);
-      return XEXP (src, 0);
-    }
+    return XEXP (src, 0);
   else if (XEXP (src, 1) == pc_rtx)
     {
       rtx cond = XEXP (src, 0);
@@ -541,14 +521,45 @@ sched_get_condition_with_rev (const_rtx insn, bool *rev)
 
       if (rev)
 	*rev = true;
-      INSN_COND (insn) = cond;
-      INSN_REVERSE_COND (insn) = true;
       return cond;
     }
 
   return 0;
 }
 
+/* Caching variant of sched_get_condition_with_rev_uncached.
+   We only do actual work the first time we come here for an insn; the
+   results are cached in INSN_CACHED_COND and INSN_REVERSE_COND.  */
+static rtx
+sched_get_condition_with_rev (const_rtx insn, bool *rev)
+{
+  if (INSN_LUID (insn) == 0)
+    return sched_get_condition_with_rev_uncached (insn, rev);
+
+  if (INSN_CACHED_COND (insn) == const_true_rtx)
+    return NULL_RTX;
+
+  if (INSN_CACHED_COND (insn) != NULL_RTX)
+    {
+      if (rev)
+	*rev = INSN_REVERSE_COND (insn);
+      return INSN_CACHED_COND (insn);
+    }
+
+  INSN_CACHED_COND (insn)
+   = sched_get_condition_with_rev_uncached (insn, &INSN_REVERSE_COND (insn));
+
+  if (INSN_CACHED_COND (insn) == NULL_RTX)
+    {
+      INSN_CACHED_COND (insn) = const_true_rtx;
+      return NULL_RTX;
+    }
+
+  if (rev)
+    *rev = INSN_REVERSE_COND (insn);
+  return INSN_CACHED_COND (insn);
+}
+
 /* True when we can find a condition under which INSN is executed.  */
 static bool
 sched_has_condition_p (const_rtx insn)
@@ -2884,9 +2895,9 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx insn)
 	      for (list = reg_last->uses; list; list = XEXP (list, 1))
 		{
 		  rtx other = XEXP (list, 0);
-		  if (INSN_COND (other) != const_true_rtx
-		      && refers_to_regno_p (i, i + 1, INSN_COND (other), NULL))
-		    INSN_COND (other) = const_true_rtx;
+		  if (INSN_CACHED_COND (other) != const_true_rtx
+		      && refers_to_regno_p (i, i + 1, INSN_CACHED_COND (other), NULL))
+		    INSN_CACHED_COND (other) = const_true_rtx;
 		}
 	    }
 	}
diff --git a/gcc/sched-int.h b/gcc/sched-int.h
index f310f8a..b8240d7 100644
--- a/gcc/sched-int.h
+++ b/gcc/sched-int.h
@@ -849,7 +849,7 @@ extern VEC(haifa_deps_insn_data_def, heap) *h_d_i_d;
 #define INSN_RESOLVED_FORW_DEPS(INSN) (HDID (INSN)->resolved_forw_deps)
 #define INSN_HARD_BACK_DEPS(INSN) (HDID (INSN)->hard_back_deps)
 #define INSN_SPEC_BACK_DEPS(INSN) (HDID (INSN)->spec_back_deps)
-#define INSN_COND(INSN)	(HDID (INSN)->cond)
+#define INSN_CACHED_COND(INSN)	(HDID (INSN)->cond)
 #define INSN_REVERSE_COND(INSN) (HDID (INSN)->reverse_cond)
 #define CANT_MOVE(INSN)	(HDID (INSN)->cant_move)
 #define CANT_MOVE_BY_LUID(LUID)	(VEC_index (haifa_deps_insn_data_def, h_d_i_d, \

[PATCH 4/8] Properly loop over all hard regs for mode

[Alexander Monakov]

From: Sergey Grechanik <mouseentity@condor.intra.ispras.ru>

There are several places where bitmap_bit_p function is used to test if some
register is in the regset.  We need to take into account the fact that
depending on mode, we need to test multiple hard regs.

2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

	* sel-sched-ir.h (register_unavailable_p): Declare.
	* sel-sched-ir.c (register_unavailable_p): New.  Use it...
	(set_unavailable_target_for_expr): ... here to properly test
	availability of a register.
	(speculate_expr): Ditto.
	* sel-sched.c (substitute_reg_in_expr): Ditto.
	(av_set_could_be_blocked_by_bookkeeping_p): Ditto.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 8f5cd3f..f22e637 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1883,7 +1883,7 @@ set_unavailable_target_for_expr (expr_t expr, regset lv_set)
   if (EXPR_SEPARABLE_P (expr))
     {
       if (REG_P (EXPR_LHS (expr))
-          && bitmap_bit_p (lv_set, REGNO (EXPR_LHS (expr))))
+          && register_unavailable_p (lv_set, EXPR_LHS (expr)))
 	{
 	  /* If it's an insn like r1 = use (r1, ...), and it exists in
 	     different forms in each of the av_sets being merged, we can't say
@@ -1904,8 +1904,8 @@ set_unavailable_target_for_expr (expr_t expr, regset lv_set)
 	     miss a unifying code motion along both branches using a renamed
 	     register, but it won't affect a code correctness since upon
 	     an actual code motion a bookkeeping code would be generated.  */
-	  if (bitmap_bit_p (VINSN_REG_USES (EXPR_VINSN (expr)),
-			    REGNO (EXPR_LHS (expr))))
+	  if (register_unavailable_p (VINSN_REG_USES (EXPR_VINSN (expr)),
+				      EXPR_LHS (expr)))
 	    EXPR_TARGET_AVAILABLE (expr) = -1;
 	  else
 	    EXPR_TARGET_AVAILABLE (expr) = false;
@@ -1971,8 +1971,8 @@ speculate_expr (expr_t expr, ds_t ds)
 
         /* Do not allow clobbering the address register of speculative
            insns.  */
-        if (bitmap_bit_p (VINSN_REG_USES (EXPR_VINSN (expr)),
-                          expr_dest_regno (expr)))
+        if (register_unavailable_p (VINSN_REG_USES (EXPR_VINSN (expr)),
+				    expr_dest_reg (expr)))
           {
             EXPR_TARGET_AVAILABLE (expr) = false;
             return 2;
@@ -2026,6 +2026,25 @@ mark_unavailable_targets (av_set_t join_set, av_set_t av_set, regset lv_set)
 }
 \f
 
+/* Returns true if REG (at least partially) is present in REGS.  */
+bool
+register_unavailable_p (regset regs, rtx reg)
+{
+  unsigned regno, end_regno;
+
+  regno = REGNO (reg);
+  if (bitmap_bit_p (regs, regno))
+    return true;
+
+  end_regno = END_REGNO (reg);
+
+  while (++regno < end_regno)
+    if (bitmap_bit_p (regs, regno))
+      return true;
+
+  return false;
+}
+
 /* Av set functions.  */
 
 /* Add a new element to av set SETP.
diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
index 838c0d1..c8f8be6 100644
--- a/gcc/sel-sched-ir.h
+++ b/gcc/sel-sched-ir.h
@@ -1573,6 +1573,7 @@ extern void sel_init_global_and_expr (bb_vec_t);
 extern void sel_finish_global_and_expr (void);
 
 extern regset compute_live (insn_t);
+extern bool register_unavailable_p (regset, rtx);
 
 /* Dependence analysis functions.  */
 extern void sel_clear_has_dependence (void);
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index e791e4c..f11faca 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -794,8 +794,8 @@ substitute_reg_in_expr (expr_t expr, insn_t insn, bool undo)
 	  /* Do not allow clobbering the address register of speculative
              insns.  */
 	  if ((EXPR_SPEC_DONE_DS (expr) & SPECULATIVE)
-              && bitmap_bit_p (VINSN_REG_USES (EXPR_VINSN (expr)),
-			       expr_dest_regno (expr)))
+              && register_unavailable_p (VINSN_REG_USES (EXPR_VINSN (expr)),
+					 expr_dest_reg (expr)))
 	    EXPR_TARGET_AVAILABLE (expr) = false;
 
 	  return true;
@@ -3631,12 +3631,12 @@ av_set_could_be_blocked_by_bookkeeping_p (av_set_t orig_ops, void *static_params
      renaming.  Check with the right register instead.  */
   if (sparams->dest && REG_P (sparams->dest))
     {
-      unsigned regno = REGNO (sparams->dest);
+      rtx reg = sparams->dest;
       vinsn_t failed_vinsn = INSN_VINSN (sparams->failed_insn);
 
-      if (bitmap_bit_p (VINSN_REG_SETS (failed_vinsn), regno)
-	  || bitmap_bit_p (VINSN_REG_USES (failed_vinsn), regno)
-	  || bitmap_bit_p (VINSN_REG_CLOBBERS (failed_vinsn), regno))
+      if (register_unavailable_p (VINSN_REG_SETS (failed_vinsn), reg)
+	  || register_unavailable_p (VINSN_REG_USES (failed_vinsn), reg)
+	  || register_unavailable_p (VINSN_REG_CLOBBERS (failed_vinsn), reg))
 	return true;
     }
 

[PATCH 5/8] Drop an incorrect assert

[Alexander Monakov]

From: Dmitry Melnik <dm@ispras.ru>

This fixes a bug caused by trying to initialize BB_AV_SET of a newly generated
jump.  It assumes that jump is only generated in new bb, while it can be
replaced in same BB by try_redirect_by_replacing_jump, if jump was conditional
and was pointing to the same BB by its both edges (this may happen due to
predication, when moving all insns up from a "diamond").

2011-08-04  Dmitry Melnik  <dm@ispras.ru>

	* sel-sched-ir.c (invalidate_av_set): Remove the assert.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index f22e637..dd8b3da 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -4284,14 +4284,13 @@ free_lv_sets (void)
       free_lv_set (bb);
 }
 
-/* Initialize an invalid AV_SET for BB.
-   This set will be updated next time compute_av () process BB.  */
+/* Mark AV_SET for BB as invalid, so this set will be updated the next time
+   compute_av() processes BB.  This function is called when creating new basic
+   blocks, as well as for blocks (either new or existing) where new jumps are
+   created when the control flow is being updated.  */
 static void
 invalidate_av_set (basic_block bb)
 {
-  gcc_assert (BB_AV_LEVEL (bb) <= 0
-	      && BB_AV_SET (bb) == NULL);
-
   BB_AV_LEVEL (bb) = -1;
 }
 

[PATCH 3/8] Fix usage of hard_regno_nregs before reload

[Alexander Monakov]

From: Sergey Grechanik <mouseentity@gmail.com>

This fixes one place where hard_regno_nregs is incorrectly guarded by
reload_completed (as if before reload all regs are pseudos).

2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

	* sel-sched.c (verify_target_availability): Fix usage of
	hard_regno_nregs.

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 92ba222..e791e4c 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -1581,7 +1581,7 @@ verify_target_availability (expr_t expr, regset used_regs,
   regno = expr_dest_regno (expr);
   mode = GET_MODE (EXPR_LHS (expr));
   target_available = EXPR_TARGET_AVAILABLE (expr) == 1;
-  n = reload_completed ? hard_regno_nregs[regno][mode] : 1;
+  n = HARD_REGISTER_NUM_P (regno) ? hard_regno_nregs[regno][mode] : 1;
 
   live_available = hard_available = true;
   for (i = 0; i < n; i++)

[PATCH 8/8] Only merge deps status for true dependencies

[Alexander Monakov]

From: Sergey Grechanik <mouseentity@condor.intra.ispras.ru>

This patch avoids changing speculative bits of a register use when it is moved
up above a speculation check where that register is used as the address
register.  We should only call ds_full_merge when the producer (speculation
check) writes to a register.

(A similar check was in place when selective scheduler was in active
development, but was dropped before it was merged, probably by accident)

2011-08-04  Sergey Grechanik  <mouseentity@ispras.ru>

	* sel-sched-ir.c (has_dependence_note_reg_use): Call ds_full_merge
	only if producer writes to the register given by regno.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 91f9dd9..745fcc1 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -3227,7 +3227,10 @@ has_dependence_note_reg_use (int regno)
 	  pro_spec_checked_ds = INSN_SPEC_CHECKED_DS (has_dependence_data.pro);
 	  pro_spec_checked_ds = ds_get_max_dep_weak (pro_spec_checked_ds);
 
-	  if (pro_spec_checked_ds != 0)
+	  if (pro_spec_checked_ds != 0
+	      /* FIXME: if mode for REGNO was available here, we could use
+	         register_unavailable_p that tests all hard regs for mode.  */
+	      && bitmap_bit_p (INSN_REG_SETS (has_dependence_data.pro), regno))
 	    /* Merge BE_IN_SPEC bits into *DSP.  */
 	    *dsp = ds_full_merge (*dsp, pro_spec_checked_ds,
 				  NULL_RTX, NULL_RTX);

[PING] Selective scheduler fixes

[Alexander Monakov]

Ping

On Wed, 3 Aug 2011, Alexander Monakov wrote:

> Hello,
> 
> This is a series of selective scheduler bug fixes.  They fix problems that
> have been discovered during internal testing, and one patch is necessary as
> preparation to predication support in the selective scheduler (predication
> patches will be submitted separately later).
> 
> I'm sorry that patches come without testcases, but as most of the problems
> have been discovered on a patched scheduler, it's virtually impossible to
> provide testcases that would fail without a patch and pass with it.
> 
> The patches have been bootstrapped and regtested together on x86-64 and ia64
> (without java on ia64).  Additionally, a number of tests was done with arm
> cross-compiler.  I'll commit approved patches separately.
> 
> OK for trunk?
> 
> Dmitry Melnik (3):
>   Take maximum spec when merging exprs
>   Make more insns unique
>   Drop an incorrect assert
> 
> Sergey Grechanik (5):
>   Fix usage of hard_regno_nregs before reload
>   Properly loop over all hard regs for mode
>   Try successors to find seqno
>   Factor out caching logic for INSN_COND
>   Only merge deps status for true dependencies
> 
>  gcc/sched-deps.c   |   69 +++++++++++++++++++-------------
>  gcc/sched-int.h    |    2 +-
>  gcc/sel-sched-ir.c |  111 +++++++++++++++++++++++++++++++++++++++++-----------
>  gcc/sel-sched-ir.h |    1 +
>  gcc/sel-sched.c    |   14 +++---
>  5 files changed, 137 insertions(+), 60 deletions(-)
> 

Re: [PATCH 1/8] Take maximum spec when merging exprs

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Dmitry Melnik<dm@ispras.ru>
>
> EXPR_SPEC is an indicator of the speculativeness of an expression (an
> instruction or just an rhs), as it is incremented each time the expression is
> moved up across a conditional branch.  When merging expr attributes for
> similar exprs available from two destinations of a branch, sel-sched assigns
> the minimum of EXPR_SPEC's to the result, effectively making the resulting
> expr non-speculative if only one of those exprs was non-speculative.  However,
> since we are relying on EXPR_SPEC being a correct indication of expr's
> speculativeness when deciding whether it would need a bookkeeping copy, we
> really want to avoid that.
>
> The patch changes minimum to maximum, making the code match what was
> originally intended.
>
> 2011-08-04  Dmitry Melnik<dm@ispras.ru>
>
> 	* sel-sched-ir.c (merge_expr_data): Take maximum spec.
OK, thanks.

Re: [PATCH 2/8] Make more insns unique

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Dmitry Melnik<dm@ispras.ru>
>
> This patch prevents duplicating (as bookkeeping code) instructions that are
> either volatile or recognized by cannot_copy_insn_p target hook (in addition
> to already present restrictions).  This avoids generating incorrect assembler
> with duplicate labels on ARM.
>
> 2011-08-04  Dmitry Melnik<dm@ispras.ru>
>
> 	* sel-sched-ir.c (init_global_and_expr_for_insn): Forbid copying of
> 	recognized by cannot_copy_insn_p hook and volatile instructions.
OK, thanks.

Re: [PATCH 3/8] Fix usage of hard_regno_nregs before reload

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Sergey Grechanik<mouseentity@gmail.com>
>
> This fixes one place where hard_regno_nregs is incorrectly guarded by
> reload_completed (as if before reload all regs are pseudos).
>
> 2011-08-04  Sergey Grechanik<mouseentity@ispras.ru>
>
> 	* sel-sched.c (verify_target_availability): Fix usage of
> 	hard_regno_nregs.
>
Ok, thanks.

Re: [PATCH 4/8] Properly loop over all hard regs for mode

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Sergey Grechanik<mouseentity@condor.intra.ispras.ru>
>
> There are several places where bitmap_bit_p function is used to test if some
> register is in the regset.  We need to take into account the fact that
> depending on mode, we need to test multiple hard regs.
>
> 2011-08-04  Sergey Grechanik<mouseentity@ispras.ru>
>
> 	* sel-sched-ir.h (register_unavailable_p): Declare.
> 	* sel-sched-ir.c (register_unavailable_p): New.  Use it...
> 	(set_unavailable_target_for_expr): ... here to properly test
> 	availability of a register.
> 	(speculate_expr): Ditto.
> 	* sel-sched.c (substitute_reg_in_expr): Ditto.
> 	(av_set_could_be_blocked_by_bookkeeping_p): Ditto.
Ok.

Re: [PATCH 5/8] Drop an incorrect assert

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Dmitry Melnik<dm@ispras.ru>
>
> This fixes a bug caused by trying to initialize BB_AV_SET of a newly generated
> jump.  It assumes that jump is only generated in new bb, while it can be
> replaced in same BB by try_redirect_by_replacing_jump, if jump was conditional
> and was pointing to the same BB by its both edges (this may happen due to
> predication, when moving all insns up from a "diamond").
>
> 2011-08-04  Dmitry Melnik<dm@ispras.ru>
>
> 	* sel-sched-ir.c (invalidate_av_set): Remove the assert.
>
Ok, thanks.

Re: [PATCH 6/8] Try successors to find seqno

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Sergey Grechanik<mouseentity@condor.intra.ispras.ru>
>
> This patch fixes a problem when new jumps created in
> sel_redirect_edge_and_branch_force could not get correct seqnos.
>
> get_seqno_of_a_pred is renamed to get_seqno_for_a_jump.
> Implementation-wise, it supports looking at multiple predecessors, and, if
> that fails, also at successors.  get_seqno_by_succs is implemented similar to
> get_seqno_by_preds.
>
> 2011-08-04  Sergey Grechanik<mouseentity@ispras.ru>
>
> 	* sel-sched-ir.c (get_seqno_of_a_pred): Rename to
> 	get_seqno_for_a_jump.  Update the caller.
> 	(get_seqno_by_succs): New.  Use it ...
> 	(get_seqno_for_a_jump): ... here to find a seqno if looking at
> 	predecessors was not sufficient.
> 	(get_seqno_by_preds): Include head in iteration range, exclude insn.
Ok, thanks.

Re: [PATCH 7/8] Factor out caching logic for INSN_COND

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Sergey Grechanik<mouseentity@condor.intra.ispras.ru>
>
> Sometimes we need to be able to call sched_get_condition_with_rev from
> selective scheduler for an instruction with zero luid (i.e. before h_d_i_d
> had been extended).  On such occasion, we need to bypass the caching and use
> "old", uncached lookup.
>
> The patch factors out caching logic to a separate function and amends it to
> skip cached lookup for instructions with zero luid.  It also renames INSN_COND
> to INSN_CACHED_COND to avoid clash with the same macro in predication patch
> for sel-sched.
>
> 2011-08-04  Sergey Grechanik<mouseentity@ispras.ru>
>
> 	* sched-deps.c (sched_get_condition_with_rev): Rename to ...
> 	(sched_get_condition_with_rev_uncached): ... this.  Factor out
> 	condition caching logic into ...
> 	(sched_get_condition_with_rev): ... this.  Reimplement.  Do not
> 	attempt to use cache for instructions with zero luid.
> 	(sched_analyze_insn): Use INSN_CACHED_COND instead of INSN_COND.
> 	* sched-int.h (INSN_COND): Rename to INSN_CACHED_COND.
Ok, thanks.

Re: [PATCH 8/8] Only merge deps status for true dependencies

[Vladimir Makarov]

On 08/03/2011 11:30 AM, Alexander Monakov wrote:
> From: Sergey Grechanik<mouseentity@condor.intra.ispras.ru>
>
> This patch avoids changing speculative bits of a register use when it is moved
> up above a speculation check where that register is used as the address
> register.  We should only call ds_full_merge when the producer (speculation
> check) writes to a register.
>
> (A similar check was in place when selective scheduler was in active
> development, but was dropped before it was merged, probably by accident)
>
> 2011-08-04  Sergey Grechanik<mouseentity@ispras.ru>
>
> 	* sel-sched-ir.c (has_dependence_note_reg_use): Call ds_full_merge
> 	only if producer writes to the register given by regno.
>
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index 91f9dd9..745fcc1 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -3227,7 +3227,10 @@ has_dependence_note_reg_use (int regno)
>   	  pro_spec_checked_ds = INSN_SPEC_CHECKED_DS (has_dependence_data.pro);
>   	  pro_spec_checked_ds = ds_get_max_dep_weak (pro_spec_checked_ds);
>
> -	  if (pro_spec_checked_ds != 0)
> +	  if (pro_spec_checked_ds != 0
> +	      /* FIXME: if mode for REGNO was available here, we could use
> +	         register_unavailable_p that tests all hard regs for mode.  */
I don't think you need the mode here.  For pseudo register it is always 
1. For hard registers note_reg_use is always called for every hard 
register containing a value.

So the patch is ok, if you remove the comment or write what I mentioned 
as the comment.

Thanks.
> +	&&  bitmap_bit_p (INSN_REG_SETS (has_dependence_data.pro), regno))
>   	    /* Merge BE_IN_SPEC bits into *DSP.  */
>   	    *dsp = ds_full_merge (*dsp, pro_spec_checked_ds,
>   				  NULL_RTX, NULL_RTX);