Re: [PR48866] three alternative fixes

[Alexandre Oliva <aoliva at redhat dot com>]

On May 30, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> 1. emit debug temps for replaceable DEFs that end up being referenced in
> debug insns.  We already have some code to try to deal with this, but it
> emits the huge expressions we'd rather avoid, and it may create
> unnecessary duplication.  This new approach emits a placeholder instead
> of skipping replaceable DEFs altogether, and then, if the DEF is
> referenced in a debug insn (perhaps during the late debug re-expasion of
> some other placeholder), it is expanded.  Placeholders that end up not
> being referenced are then throw away.

This is my favorite option, for it's safest: it doesn't change
executable code at all (or should I say it *shouldn't* change it, for I
haven't verified that it doesn't), retaining any register pressure
benefits from TER.

The drawbacks are higher likelihood of loss of debug information as
complex debug value expressions now require the recursive expansion of
multiple debug temps, which is now more likely to hit the recursion
limit.  E.g., for a chain of dereferences, before the patch we'd get
something like:

;; x_ = ****y_; (actually a sequence of replaceable gimple assignments)
;; debug x => x_
(debug_insn (var_location x (mem (mem (mem (mem (reg y_)))))))
[...]
;; ? = x_ + 1;
(set (reg) (mem (reg y_)))
(set (reg) (mem (reg...)))
(set (reg) (mem (reg...)))
(set (reg x_) (mem (reg...)))
(set (reg ?) (plus (reg x_) (const_int 1)))

After the patch, the first two stmts would expand to:

;; x_ = ****y; (replaceable sequence)
(debug_insn (var_location D#3 (mem (reg ...))))
(debug_insn (var_location D#2 (mem D#3)))
(debug_insn (var_location D#1 (mem D#2)))
;; debug x => x_
(debug_insn (var_location x (mem D#1)))

The additional debug temps will likely come at a compile-time cost,
especially during var-tracking, but this may be at least in part
recovered because the debug temps hold values that are likely to be
computed by actual insns, so var-tracking will create value tracking
entries for them anyway.


Unlike the other patches, this alternative will *not* expand
replaceable code at its natural location, so the executable code will
remain at the point of use.  Single-stepping will therefore remain just
as confusing, and since debug bind stmts will likely remain at points
one can't stop, they may remain unusable.  The debug information
extensions I proposed at the GCC Summit last year will alleviate this
issue, but I still don't have a concrete plan for completing the
implementation of that feature, so this is something to keep in mind
if this patch is preferred.

Here's the patch.  Regstrapped on x86_64-linux-gnu and i686-linux-gnu.

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/48866
	* cfgexpand.c (def_expansions): New.
	(def_expansions_init): New.
	(def_expansions_remove_placeholder, def_expansions_fini): New.
	(def_has_expansion_ptr, def_get_expansion_ptr): New.
	(expand_debug_expr): Create debug temps as needed.
	(expand_debug_insn): New, split out of...
	(expand_debug_locations): ... this.
	(gen_emit_debug_insn): New, split out of...
	(expand_gimple_basic_block): ... this.  Simplify expansion of
	debug stmts.  Emit placeholders for replaceable DEFs, rather
	than debug temps at last non-debug uses.
	(gimple_expand_cfg): Initialize and finalize expansions cache.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c.orig	2011-05-28 08:40:42.000000000 -0300
+++ gcc/cfgexpand.c	2011-05-28 11:33:22.727418563 -0300
@@ -2337,6 +2337,84 @@ convert_debug_memory_address (enum machi
   return x;
 }
 
+/* Map replaceable SSA_NAMEs to their RTL expansions.  */
+static struct pointer_map_t *def_expansions;
+
+/* Mark debug insns that are placeholders for replaceable SSA_NAMEs
+   that have not been expanded yet.  */
+#define DEBUG_INSN_TOEXPAND(RTX)					\
+  (RTL_FLAG_CHECK1("DEBUG_INSN_TOEXPAND", (RTX), DEBUG_INSN)->used)
+
+/* Initialize the def_expansions data structure.  This is to be called
+   before expansion of a function starts.  */
+
+static void
+def_expansions_init (void)
+{
+  gcc_checking_assert (!def_expansions);
+  def_expansions = pointer_map_create ();
+}
+
+/* Remove the DEBUG_INSN at **XP if it still binds an SSA_NAME.  */
+
+static bool
+def_expansions_remove_placeholder (const void *def ATTRIBUTE_UNUSED,
+				   void **xp,
+				   void *arg ATTRIBUTE_UNUSED)
+{
+  rtx insn = (rtx) *xp;
+
+  gcc_checking_assert (insn);
+
+  if (TREE_CODE (INSN_VAR_LOCATION_DECL (insn)) == SSA_NAME)
+    {
+      gcc_assert (!DEBUG_INSN_TOEXPAND (insn));
+      remove_insn (insn);
+    }
+
+  return true;
+}
+
+/* Finalize the def_expansions data structure.  This is to be called
+   at the end of the expansion of a function.  */
+
+static void
+def_expansions_fini (void)
+{
+  gcc_checking_assert (def_expansions);
+  pointer_map_traverse (def_expansions,
+			def_expansions_remove_placeholder, NULL);
+  pointer_map_destroy (def_expansions);
+  def_expansions = NULL;
+}
+
+/* Return NULL if no expansion is registered for EXP, or a pointer to
+   the rtx expanded from it otherwise.  EXP must be a replaceable
+   SSA_NAME.  */
+
+static void *const *
+def_has_expansion_ptr (tree exp)
+{
+  gcc_checking_assert (def_expansions);
+  gcc_checking_assert (TREE_CODE (exp) == SSA_NAME);
+  gcc_checking_assert (bitmap_bit_p (SA.values, SSA_NAME_VERSION (exp)));
+  return pointer_map_contains (def_expansions, exp);
+}
+
+/* Return a pointer to the rtx expanded from EXP.  EXP must be a
+   replaceable SSA_NAME.  */
+
+static void **
+def_get_expansion_ptr (tree exp)
+{
+  gcc_checking_assert (def_expansions);
+  gcc_checking_assert (TREE_CODE (exp) == SSA_NAME);
+  gcc_checking_assert (bitmap_bit_p (SA.values, SSA_NAME_VERSION (exp)));
+  return pointer_map_insert (def_expansions, exp);
+}
+
+static void expand_debug_insn (rtx insn);
+
 /* Return an RTX equivalent to the value of the tree expression
    EXP.  */
 
@@ -3131,7 +3209,35 @@ expand_debug_expr (tree exp)
 	gimple g = get_gimple_for_ssa_name (exp);
 	if (g)
 	  {
-	    op0 = expand_debug_expr (gimple_assign_rhs_to_tree (g));
+	    void *const *xp = def_has_expansion_ptr (exp);
+	    rtx insn;
+	    tree vexpr;
+
+	    gcc_checking_assert (xp);
+
+	    insn = (rtx) *xp;
+
+	    /* If this still has the original SSA_NAME, emit a debug
+	       temp and compute the RTX value.  */
+	    if (TREE_CODE (INSN_VAR_LOCATION_DECL (insn)) == SSA_NAME)
+	      {
+		tree var = SSA_NAME_VAR (INSN_VAR_LOCATION_DECL (insn));
+
+		vexpr = make_node (DEBUG_EXPR_DECL);
+		DECL_ARTIFICIAL (vexpr) = 1;
+		TREE_TYPE (vexpr) = TREE_TYPE (var);
+		DECL_MODE (vexpr) = DECL_MODE (var);
+		INSN_VAR_LOCATION_DECL (insn) = vexpr;
+
+		gcc_checking_assert (!DEBUG_INSN_TOEXPAND (insn));
+		DEBUG_INSN_TOEXPAND (insn) = 1;
+		expand_debug_insn (insn);
+	      }
+	    else
+	      vexpr = INSN_VAR_LOCATION_DECL (insn);
+
+	    op0 = expand_debug_expr (vexpr);
+
 	    if (!op0)
 	      return NULL;
 	  }
@@ -3293,6 +3399,45 @@ expand_debug_expr (tree exp)
     }
 }
 
+/* Expand the LOC value of the debug insn INSN.  */
+
+static void
+expand_debug_insn (rtx insn)
+{
+  tree value = (tree)INSN_VAR_LOCATION_LOC (insn);
+  rtx val;
+  enum machine_mode mode;
+
+  gcc_checking_assert (TREE_CODE (INSN_VAR_LOCATION_DECL (insn)) != SSA_NAME);
+  gcc_checking_assert (DEBUG_INSN_TOEXPAND (insn));
+  DEBUG_INSN_TOEXPAND (insn) = 0;
+
+  if (value == NULL_TREE)
+    val = NULL_RTX;
+  else
+    {
+      rtx last = get_last_insn ();
+      val = expand_debug_expr (value);
+      gcc_checking_assert (last == get_last_insn ());
+    }
+
+  if (!val)
+    val = gen_rtx_UNKNOWN_VAR_LOC ();
+  else
+    {
+      mode = GET_MODE (INSN_VAR_LOCATION (insn));
+
+      gcc_assert (mode == GET_MODE (val)
+		  || (GET_MODE (val) == VOIDmode
+		      && (CONST_INT_P (val)
+			  || GET_CODE (val) == CONST_FIXED
+			  || GET_CODE (val) == CONST_DOUBLE
+			  || GET_CODE (val) == LABEL_REF)));
+    }
+
+  INSN_VAR_LOCATION_LOC (insn) = val;
+}
+
 /* Expand the _LOCs in debug insns.  We run this after expanding all
    regular insns, so that any variables referenced in the function
    will have their DECL_RTLs set.  */
@@ -3301,7 +3446,6 @@ static void
 expand_debug_locations (void)
 {
   rtx insn;
-  rtx last = get_last_insn ();
   int save_strict_alias = flag_strict_aliasing;
 
   /* New alias sets while setting up memory attributes cause
@@ -3310,38 +3454,54 @@ expand_debug_locations (void)
   flag_strict_aliasing = 0;
 
   for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
-    if (DEBUG_INSN_P (insn))
-      {
-	tree value = (tree)INSN_VAR_LOCATION_LOC (insn);
-	rtx val;
-	enum machine_mode mode;
+    /* Skip debug insns that have SSA_NAMEs as bound decls.  These
+       will only have their values expanded if referenced by other
+       debug insns, and they are removed if not expanded.  */
+    if (DEBUG_INSN_P (insn) && DEBUG_INSN_TOEXPAND (insn))
+      expand_debug_insn (insn);
 
-	if (value == NULL_TREE)
-	  val = NULL_RTX;
-	else
-	  {
-	    val = expand_debug_expr (value);
-	    gcc_assert (last == get_last_insn ());
-	  }
+  flag_strict_aliasing = save_strict_alias;
+}
 
-	if (!val)
-	  val = gen_rtx_UNKNOWN_VAR_LOC ();
-	else
-	  {
-	    mode = GET_MODE (INSN_VAR_LOCATION (insn));
+/* Emit a debug insn that binds VAR to VALUE, with location and block
+   information taken from STMT.  */
 
-	    gcc_assert (mode == GET_MODE (val)
-			|| (GET_MODE (val) == VOIDmode
-			    && (CONST_INT_P (val)
-				|| GET_CODE (val) == CONST_FIXED
-				|| GET_CODE (val) == CONST_DOUBLE
-				|| GET_CODE (val) == LABEL_REF)));
-	  }
+static rtx
+gen_emit_debug_insn (tree var, tree value, gimple stmt)
+{
+  enum machine_mode mode;
+  location_t sloc = get_curr_insn_source_location ();
+  tree sblock = get_curr_insn_block ();
+  rtx val, insn;
+
+  if (DECL_P (var))
+    mode = DECL_MODE (var);
+  else if (TREE_CODE (var) == SSA_NAME)
+    mode = DECL_MODE (SSA_NAME_VAR (var));
+  else
+    mode = TYPE_MODE (TREE_TYPE (var));
 
-	INSN_VAR_LOCATION_LOC (insn) = val;
-      }
+  val = gen_rtx_VAR_LOCATION
+    (mode, var, (rtx)value, VAR_INIT_STATUS_INITIALIZED);
 
-  flag_strict_aliasing = save_strict_alias;
+  set_curr_insn_source_location (gimple_location (stmt));
+  set_curr_insn_block (gimple_block (stmt));
+  insn = emit_debug_insn (val);
+  set_curr_insn_source_location (sloc);
+  set_curr_insn_block (sblock);
+
+  DEBUG_INSN_TOEXPAND (insn) = (TREE_CODE (var) != SSA_NAME);
+
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    {
+      /* We can't dump the insn with a TREE where an RTX
+	 is expected.  */
+      PAT_VAR_LOCATION_LOC (val) = const0_rtx;
+      maybe_dump_rtl_for_gimple_stmt (stmt, PREV_INSN (insn));
+      PAT_VAR_LOCATION_LOC (val) = (rtx)value;
+    }
+
+  return insn;
 }
 
 /* Expand basic block BB from GIMPLE trees to RTL.  */
@@ -3433,104 +3593,6 @@ expand_gimple_basic_block (basic_block b
 
       stmt = gsi_stmt (gsi);
 
-      /* If this statement is a non-debug one, and we generate debug
-	 insns, then this one might be the last real use of a TERed
-	 SSA_NAME, but where there are still some debug uses further
-	 down.  Expanding the current SSA name in such further debug
-	 uses by their RHS might lead to wrong debug info, as coalescing
-	 might make the operands of such RHS be placed into the same
-	 pseudo as something else.  Like so:
-	   a_1 = a_0 + 1;   // Assume a_1 is TERed and a_0 is dead
-	   use(a_1);
-	   a_2 = ...
-           #DEBUG ... => a_1
-	 As a_0 and a_2 don't overlap in lifetime, assume they are coalesced.
-	 If we now would expand a_1 by it's RHS (a_0 + 1) in the debug use,
-	 the write to a_2 would actually have clobbered the place which
-	 formerly held a_0.
-
-	 So, instead of that, we recognize the situation, and generate
-	 debug temporaries at the last real use of TERed SSA names:
-	   a_1 = a_0 + 1;
-           #DEBUG #D1 => a_1
-	   use(a_1);
-	   a_2 = ...
-           #DEBUG ... => #D1
-	 */
-      if (MAY_HAVE_DEBUG_INSNS
-	  && SA.values
-	  && !is_gimple_debug (stmt))
-	{
-	  ssa_op_iter iter;
-	  tree op;
-	  gimple def;
-
-	  location_t sloc = get_curr_insn_source_location ();
-	  tree sblock = get_curr_insn_block ();
-
-	  /* Look for SSA names that have their last use here (TERed
-	     names always have only one real use).  */
-	  FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
-	    if ((def = get_gimple_for_ssa_name (op)))
-	      {
-		imm_use_iterator imm_iter;
-		use_operand_p use_p;
-		bool have_debug_uses = false;
-
-		FOR_EACH_IMM_USE_FAST (use_p, imm_iter, op)
-		  {
-		    if (gimple_debug_bind_p (USE_STMT (use_p)))
-		      {
-			have_debug_uses = true;
-			break;
-		      }
-		  }
-
-		if (have_debug_uses)
-		  {
-		    /* OP is a TERed SSA name, with DEF it's defining
-		       statement, and where OP is used in further debug
-		       instructions.  Generate a debug temporary, and
-		       replace all uses of OP in debug insns with that
-		       temporary.  */
-		    gimple debugstmt;
-		    tree value = gimple_assign_rhs_to_tree (def);
-		    tree vexpr = make_node (DEBUG_EXPR_DECL);
-		    rtx val;
-		    enum machine_mode mode;
-
-		    set_curr_insn_source_location (gimple_location (def));
-		    set_curr_insn_block (gimple_block (def));
-
-		    DECL_ARTIFICIAL (vexpr) = 1;
-		    TREE_TYPE (vexpr) = TREE_TYPE (value);
-		    if (DECL_P (value))
-		      mode = DECL_MODE (value);
-		    else
-		      mode = TYPE_MODE (TREE_TYPE (value));
-		    DECL_MODE (vexpr) = mode;
-
-		    val = gen_rtx_VAR_LOCATION
-			(mode, vexpr, (rtx)value, VAR_INIT_STATUS_INITIALIZED);
-
-		    emit_debug_insn (val);
-
-		    FOR_EACH_IMM_USE_STMT (debugstmt, imm_iter, op)
-		      {
-			if (!gimple_debug_bind_p (debugstmt))
-			  continue;
-
-			FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
-			  SET_USE (use_p, vexpr);
-
-			update_stmt (debugstmt);
-		      }
-		  }
-	      }
-	  set_curr_insn_source_location (sloc);
-	  set_curr_insn_block (sblock);
-	}
-
       currently_expanding_gimple_stmt = stmt;
 
       /* Expand this statement, then evaluate the resulting RTL and
@@ -3543,64 +3605,15 @@ expand_gimple_basic_block (basic_block b
 	}
       else if (gimple_debug_bind_p (stmt))
 	{
-	  location_t sloc = get_curr_insn_source_location ();
-	  tree sblock = get_curr_insn_block ();
-	  gimple_stmt_iterator nsi = gsi;
-
-	  for (;;)
-	    {
-	      tree var = gimple_debug_bind_get_var (stmt);
-	      tree value;
-	      rtx val;
-	      enum machine_mode mode;
-
-	      if (gimple_debug_bind_has_value_p (stmt))
-		value = gimple_debug_bind_get_value (stmt);
-	      else
-		value = NULL_TREE;
+	  tree var = gimple_debug_bind_get_var (stmt);
+	  tree value;
 
-	      last = get_last_insn ();
-
-	      set_curr_insn_source_location (gimple_location (stmt));
-	      set_curr_insn_block (gimple_block (stmt));
-
-	      if (DECL_P (var))
-		mode = DECL_MODE (var);
-	      else
-		mode = TYPE_MODE (TREE_TYPE (var));
-
-	      val = gen_rtx_VAR_LOCATION
-		(mode, var, (rtx)value, VAR_INIT_STATUS_INITIALIZED);
-
-	      emit_debug_insn (val);
-
-	      if (dump_file && (dump_flags & TDF_DETAILS))
-		{
-		  /* We can't dump the insn with a TREE where an RTX
-		     is expected.  */
-		  PAT_VAR_LOCATION_LOC (val) = const0_rtx;
-		  maybe_dump_rtl_for_gimple_stmt (stmt, last);
-		  PAT_VAR_LOCATION_LOC (val) = (rtx)value;
-		}
-
-	      /* In order not to generate too many debug temporaries,
-	         we delink all uses of debug statements we already expanded.
-		 Therefore debug statements between definition and real
-		 use of TERed SSA names will continue to use the SSA name,
-		 and not be replaced with debug temps.  */
-	      delink_stmt_imm_use (stmt);
-
-	      gsi = nsi;
-	      gsi_next (&nsi);
-	      if (gsi_end_p (nsi))
-		break;
-	      stmt = gsi_stmt (nsi);
-	      if (!gimple_debug_bind_p (stmt))
-		break;
-	    }
+	  if (gimple_debug_bind_has_value_p (stmt))
+	    value = gimple_debug_bind_get_value (stmt);
+	  else
+	    value = NULL_TREE;
 
-	  set_curr_insn_source_location (sloc);
-	  set_curr_insn_block (sblock);
+	  gen_emit_debug_insn (var, value, stmt);
 	}
       else
 	{
@@ -3621,14 +3634,29 @@ expand_gimple_basic_block (basic_block b
 	      def_operand_p def_p;
 	      def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF);
 
-	      if (def_p != NULL)
+	      /* If this stmt is in the list of replaceable
+		 expressions, expand a placeholder for a debug insn.  */
+	      if (def_p != NULL && SA.values
+		  && bitmap_bit_p (SA.values,
+				   SSA_NAME_VERSION (DEF_FROM_PTR (def_p))))
 		{
-		  /* Ignore this stmt if it is in the list of
-		     replaceable expressions.  */
-		  if (SA.values
-		      && bitmap_bit_p (SA.values,
-				       SSA_NAME_VERSION (DEF_FROM_PTR (def_p))))
+		  tree def = DEF_FROM_PTR (def_p);
+		  void **xp;
+
+		  gcc_checking_assert (TREE_CODE (def) == SSA_NAME
+				       && DECL_P (SSA_NAME_VAR (def)));
+
+		  if (!MAY_HAVE_DEBUG_INSNS)
 		    continue;
+
+		  xp = def_get_expansion_ptr (def);
+		  gcc_checking_assert (!*xp);
+		  *xp = last
+		    = gen_emit_debug_insn (def,
+					   gimple_assign_rhs_to_tree (stmt),
+					   stmt);
+
+		  continue;
 		}
 	      last = expand_gimple_stmt (stmt);
 	      maybe_dump_rtl_for_gimple_stmt (stmt, last);
@@ -3970,6 +3998,7 @@ gimple_expand_cfg (void)
   rtx var_seq;
   unsigned i;
 
+
   timevar_push (TV_OUT_OF_SSA);
   rewrite_out_of_ssa (&SA);
   timevar_pop (TV_OUT_OF_SSA);
@@ -4112,11 +4141,18 @@ gimple_expand_cfg (void)
     e->flags &= ~EDGE_EXECUTABLE;
 
   lab_rtx_for_bb = pointer_map_create ();
+
+  if (MAY_HAVE_DEBUG_INSNS)
+    def_expansions_init ();
+
   FOR_BB_BETWEEN (bb, init_block->next_bb, EXIT_BLOCK_PTR, next_bb)
     bb = expand_gimple_basic_block (bb);
 
   if (MAY_HAVE_DEBUG_INSNS)
-    expand_debug_locations ();
+    {
+      expand_debug_locations ();
+      def_expansions_fini ();
+    }
 
   execute_free_datastructures ();
   timevar_push (TV_OUT_OF_SSA);


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer