[0/3] Improve debug info for addressable vars

[0/3] Improve debug info for addressable vars

[Richard Sandiford]

Taking the address of a variable stops us doing var-tracking on it,
so that we just use the DECL_RTL instead.  This can easily cause wrong
debug info for regions of code that would have had correct debug info
if the variable weren't addressable.  E.g.:

{
  int base;
  get_start (&base);
  x[i1] = base;
  base += 1; // No need to store this
  x[i2] = base; // ...so the debug info for "base" is wrong here
}

or (the motivating example):

{
  int base;
  get_start (&base);
  for (int i = 0; i < n; ++i)
    {
      x[i] = base;
      base += y[i]; // Can apply LSM here, so the debug info for "base"
                    // in the loop is wrong
    }
  consume (&base);
}

This patch series lets us use the DECL_RTL location for some parts of a
variable's lifetime and debug-bind locations for other parts:

1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL.  The binding
   holds until overridden.

2) RTL does the same thing using:

     (var_location VAR (decl_rtl_ref VAR))

   where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
   by reference rather than by value.

   We can't just use "(var_location VAR (mem X))" for this, because
   that would bind VAR to the value that (mem X) has at that exact point.
   VAR would therefore get reset by any possible change to (mem X),
   whereas here we want it to track (possibly indirect) updates instead.

3) The gimplifier decides which variables should get the new treatment
   and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
   Clobbers continue to mark the end of VAR's lifetime.

4) Stores to VAR implicitly reestablish the link between VAR and its
   DECL_RTL.  This is simpler (and IMO more robust) than inserting an
   explicit "VAR s=> VAR" at every write.

5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
   falling back to a "VAR => NULL" reset if that fails.

Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
and patch 3 uses it for LSM.

Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
OK to install?

Richard

[1/3] Add a DECL_RTL_REF rtx code

[Richard Sandiford]

This patch adds a new rtx code for capturing a decl's DECL_RTL by
reference rather than by value.  When used in a VAR_LOCATION, the
VAR_LOCATION continues to track the decl as it changes over time,
rather than "remembering" the value that the decl had at the point
of the VAR_LOCATION.

2019-06-01  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* rtl.def (DEF_RTL_EXPR): New rtx code.
	* doc/rtl.texi: Document it.
	* rtl.h (DECL_RTL_REF_TARGET): New macro.
	* rtl.c (rtx_equal_p_cb, rtx_equal_p): Handle DEF_RTL_EXPR.
	* alias.c (rtx_equal_for_memref_p): Likewise.
	* cselib.c (invariant_or_equiv_p, rtx_equal_for_cselib_1)
	(cselib_hash_rtx): Likewise.
	* dwarf2out.c (mem_loc_descriptor): Likewise.
	* print-rtl.c (rtx_writer::print_rtx_operand): Likewise.

Index: gcc/rtl.def
===================================================================
--- gcc/rtl.def	2019-05-31 17:27:35.155089238 +0100
+++ gcc/rtl.def	2019-06-01 16:38:31.985677188 +0100
@@ -775,6 +775,11 @@ DEF_RTL_EXPR(DEBUG_IMPLICIT_PTR, "debug_
    parameter.  */
 DEF_RTL_EXPR(ENTRY_VALUE, "entry_value", "0", RTX_OBJ)
 
+/* Captures the DECL_RTL of a DECL by reference rather than by value,
+   so that it tracks the DECL_RTL as it evolves over time.  The single
+   argument is the DECL, accessed via DECL_RTL_REF_TARGET.  */
+DEF_RTL_EXPR(DECL_RTL_REF, "decl_rtl_ref", "t", RTX_OBJ)
+
 /* Used in VAR_LOCATION for a reference to a parameter that has
    been optimized away completely.  */
 DEF_RTL_EXPR(DEBUG_PARAMETER_REF, "debug_parameter_ref", "t", RTX_OBJ)
Index: gcc/doc/rtl.texi
===================================================================
--- gcc/doc/rtl.texi	2019-03-08 18:14:25.581010702 +0000
+++ gcc/doc/rtl.texi	2019-06-01 16:38:31.969677234 +0100
@@ -3675,6 +3675,14 @@ Stands for the location of a @var{decl}
 Stands for the value a @var{decl} had at the entry point of the
 containing function.
 
+@findex decl_rtl_ref
+@item (decl_rtl_ref:@var{mode} @var{decl})
+Captures @samp{DECL_RTL (@var{decl})} by reference rather than by value,
+so that it tracks the @code{DECL_RTL} as it evolves over time.  This is
+useful for describing the debug location of a variable that spends part
+of its lifetime in memory and that can be indirectly modified while
+stored in memory.
+
 @findex debug_parameter_ref
 @item (debug_parameter_ref:@var{mode} @var{decl})
 Refers to a parameter that was completely optimized out.
@@ -4018,11 +4026,20 @@ temporaries and determining expressions
 value of each user variable at as many points (ranges, actually) in the
 program as possible.
 
-Unlike @code{NOTE_INSN_VAR_LOCATION}, the value expression in an
-@code{INSN_VAR_LOCATION} denotes a value at that specific point in the
-program, rather than an expression that can be evaluated at any later
-point before an overriding @code{VAR_LOCATION} is encountered.  E.g.,
-if a user variable is bound to a @code{REG} and then a subsequent insn
+@findex decl_rtl_ref
+@code{INSN_VAR_LOCATION} and @code{NOTE_INSN_VAR_LOCATION} differ in the
+way that they ``capture'' potentially-variable parts of an expression.
+An @code{INSN_VAR_LOCATION} captures any potentially-variable subexpression
+@emph{by value} unless it contains an explicit by-reference capture
+such as @code{decl_rtl_ref}.  A @code{NOTE_INSN_VAR_LOCATION}
+instead captures @emph{every} subexpression by reference.
+
+Capturing a subexpression @var{x} by value means that the subexpression
+represents the value @var{x} had at that particular point in time.
+Capturing by reference means that the subexpression tracks the value
+of @var{x} as it evolves over time.
+
+E.g., if a user variable is bound to a @code{REG} and then a subsequent insn
 modifies the @code{REG}, the note location would keep mapping the user
 variable to the register across the insn, whereas the insn location
 would keep the variable bound to the value, so that the variable
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	2019-05-29 10:49:39.892700893 +0100
+++ gcc/rtl.h	2019-06-01 16:38:31.985677188 +0100
@@ -1608,6 +1608,9 @@ #define REG_NOTES(INSN)	XEXP(INSN, 6)
    question.  */
 #define ENTRY_VALUE_EXP(RTX) (RTL_CHECKC1 (RTX, 0, ENTRY_VALUE).rt_rtx)
 
+/* The decl referenced by a DECL_RTL_REF.  */
+#define DECL_RTL_REF_TARGET(RTX) (RTL_CHECKC1 (RTX, 0, DECL_RTL_REF).rt_tree)
+
 enum reg_note
 {
 #define DEF_REG_NOTE(NAME) NAME,
Index: gcc/rtl.c
===================================================================
--- gcc/rtl.c	2019-04-26 10:59:07.730195479 +0100
+++ gcc/rtl.c	2019-06-01 16:38:31.985677188 +0100
@@ -486,6 +486,9 @@ rtx_equal_p_cb (const_rtx x, const_rtx y
     case ENTRY_VALUE:
       return rtx_equal_p_cb (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y), cb);
 
+    case DECL_RTL_REF:
+      return DECL_RTL_REF_TARGET (x) == DECL_RTL_REF_TARGET (y);
+
     default:
       break;
     }
@@ -628,6 +631,9 @@ rtx_equal_p (const_rtx x, const_rtx y)
     case ENTRY_VALUE:
       return rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y));
 
+    case DECL_RTL_REF:
+      return DECL_RTL_REF_TARGET (x) == DECL_RTL_REF_TARGET (y);
+
     default:
       break;
     }
Index: gcc/alias.c
===================================================================
--- gcc/alias.c	2019-05-29 10:49:39.520701975 +0100
+++ gcc/alias.c	2019-06-01 16:38:31.969677234 +0100
@@ -1769,6 +1769,9 @@ rtx_equal_for_memref_p (const_rtx x, con
       /* This is magic, don't go through canonicalization et al.  */
       return rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y));
 
+    case DECL_RTL_REF:
+      return DECL_RTL_REF_TARGET (x) == DECL_RTL_REF_TARGET (y);
+
     case VALUE:
     CASE_CONST_UNIQUE:
       /* Pointer equality guarantees equality for these nodes.  */
Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c	2019-03-08 18:15:33.700751752 +0000
+++ gcc/cselib.c	2019-06-01 16:38:31.969677234 +0100
@@ -466,6 +466,7 @@ invariant_or_equiv_p (cselib_val *v)
       if (GET_CODE (v->locs->loc) == DEBUG_EXPR
 	  || GET_CODE (v->locs->loc) == DEBUG_IMPLICIT_PTR
 	  || GET_CODE (v->locs->loc) == ENTRY_VALUE
+	  || GET_CODE (v->locs->loc) == DECL_RTL_REF
 	  || GET_CODE (v->locs->loc) == DEBUG_PARAMETER_REF)
 	return true;
 
@@ -952,6 +953,9 @@ rtx_equal_for_cselib_1 (rtx x, rtx y, ma
 	 use rtx_equal_for_cselib_1 to compare the operands.  */
       return rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y));
 
+    case DECL_RTL_REF:
+      return DECL_RTL_REF_TARGET (x) == DECL_RTL_REF_TARGET (y);
+
     case LABEL_REF:
       return label_ref_label (x) == label_ref_label (y);
 
@@ -1126,6 +1130,11 @@ cselib_hash_rtx (rtx x, int create, mach
 	hash += cselib_hash_rtx (ENTRY_VALUE_EXP (x), create, memmode);
       return hash ? hash : (unsigned int) ENTRY_VALUE;
 
+    case DECL_RTL_REF:
+      hash += ((unsigned) DECL_RTL_REF << 7)
+	      + DECL_UID (DECL_RTL_REF_TARGET (x));
+      return hash ? hash : (unsigned int) DECL_RTL_REF;
+
     case CONST_INT:
       hash += ((unsigned) CONST_INT << 7) + UINTVAL (x);
       return hash ? hash : (unsigned int) CONST_INT;
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	2019-05-29 10:49:38.836703961 +0100
+++ gcc/dwarf2out.c	2019-06-01 16:38:31.977677211 +0100
@@ -15855,6 +15855,10 @@ mem_loc_descriptor (rtx rtl, machine_mod
       mem_loc_result->dw_loc_oprnd1.v.val_loc = op0;
       break;
 
+    case DECL_RTL_REF:
+      return mem_loc_descriptor (DECL_RTL (DECL_RTL_REF_TARGET (rtl)),
+				 mode, mem_mode, initialized);
+
     case DEBUG_PARAMETER_REF:
       mem_loc_result = parameter_ref_descriptor (rtl);
       break;
Index: gcc/print-rtl.c
===================================================================
--- gcc/print-rtl.c	2019-03-08 18:15:33.660751905 +0000
+++ gcc/print-rtl.c	2019-06-01 16:38:31.981677200 +0100
@@ -678,6 +678,8 @@ rtx_writer::print_rtx_operand (const_rtx
 	print_mem_expr (m_outfile, DEBUG_IMPLICIT_PTR_DECL (in_rtx));
       else if (idx == 0 && GET_CODE (in_rtx) == DEBUG_PARAMETER_REF)
 	print_mem_expr (m_outfile, DEBUG_PARAMETER_REF_DECL (in_rtx));
+      else if (idx == 0 && GET_CODE (in_rtx) == DECL_RTL_REF)
+	print_mem_expr (m_outfile, DECL_RTL_REF_TARGET (in_rtx));
       else
 	dump_addr (m_outfile, " ", XTREE (in_rtx, idx));
 #endif

[2/3] Track debug locations of some memory variables

[Richard Sandiford]

This patch tries to track the debug location of memory variables
that have a target_for_debug_bind (and thus have a type that
satisfies is_gimple_reg_type).  For each such variable V:

* "# DEBUG V s=> V" marks the start of V's lifetime

* during V's lifetime, "# DEBUG V => X" records that the value of V is
  given by X instead of DECL_RTL (V).

* each assignment to V implicitly reestablishes the link between V
  and DECL_RTL (V).

* as before, "V = {CLOBBER}" marks the end of V's lifetime.

When removing "V = X", gsi_remove tries to insert the equivalent of
"# DEBUG V => X" at the place of the original assignment, falling back
on a "# DEBUG V => NULL" reset if that fails.

The change to insert_debug_temp_for_var_def is mostly reindentation.

The patch only handles VAR_DECLs, but we might want to extend it
to PARM_DECLs too.


2019-06-01  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* tree-core.h (tree_decl_with_vis): Group bitfields into bytes.
	(tree_decl_with_vis::track_direct_refs_for_debug_p): New field.
	* tree-streamer-in.c (unpack_ts_decl_with_vis_value_fields): Handle it.
	* tree-streamer-out.c (pack_ts_decl_with_vis_value_fields): Likewise.
	* tree.h (DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P): New macro.
	(track_direct_refs_for_debug_p): New function.
	* cfgexpand.c (expand_debug_locations): Use DECL_RTL_REF to
	handle VAR s=> VAR.
	(expand_gimple_basic_block): Insert the initialized equivalent
	of VAR s=> VAR after each direct assignment to VAR.
	* gimplify.c (scoped_possible_mem_var_p): New function, split out
	from...
	(gimplify_bind_expr): ...here.  Decide whether it would be worth
	tracking the location of a non-gimple-reg VAR_DECL using debug
	stmts and insns.  Mark them with DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P
	if so and use gimple_debug_source_binds to mark the start of their
	lifetime.
	* gimple-low.c (lower_stmt): Allow gimple_debug_source_binds
	before lowering.
	* tree-cfg.c (make_blocks): Handle gimple_debug_source_binds.
	* tree-ssa.c (insert_debug_temp_for_var_def): Handle VAR_DECLs
	as well as SSA_NAMEs.
	(insert_debug_temps_for_defs): Insert gimple_debug_binds for
	VAR_DECLs for which DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P is set.

gcc/testsuite/
	* gcc.dg/guality/addr-taken-1.c: New test.
	* gcc.dg/guality/addr-taken-2.c: Likewise.

Index: gcc/tree-core.h
===================================================================
--- gcc/tree-core.h	2019-05-29 10:49:39.896700883 +0100
+++ gcc/tree-core.h	2019-06-01 16:38:35.421667415 +0100
@@ -1761,13 +1761,12 @@ struct GTY(()) tree_decl_with_vis {
  unsigned dllimport_flag : 1;
  /* Don't belong to VAR_DECL exclusively.  */
  unsigned weak_flag : 1;
-
  unsigned seen_in_bind_expr : 1;
+
  unsigned comdat_flag : 1;
  /* Used for FUNCTION_DECL, VAR_DECL and in C++ for TYPE_DECL.  */
  ENUM_BITFIELD(symbol_visibility) visibility : 2;
  unsigned visibility_specified : 1;
-
  /* Belong to FUNCTION_DECL exclusively.  */
  unsigned init_priority_p : 1;
  /* Used by C++ only.  Might become a generic decl flag.  */
@@ -1776,11 +1775,16 @@ struct GTY(()) tree_decl_with_vis {
  unsigned cxx_constructor : 1;
  /* Belong to FUNCTION_DECL exclusively.  */
  unsigned cxx_destructor : 1;
+
  /* Belong to FUNCTION_DECL exclusively.  */
  unsigned final : 1;
  /* Belong to FUNCTION_DECL exclusively.  */
  unsigned regdecl_flag : 1;
- /* 14 unused bits. */
+ /* Records whether direct references to VAR_DECLs should be tracked via
+    debug stmts.  */
+ unsigned track_direct_refs_for_debug_p : 1;
+
+ /* 13 unused bits. */
 };
 
 struct GTY(()) tree_var_decl {
Index: gcc/tree-streamer-in.c
===================================================================
--- gcc/tree-streamer-in.c	2019-05-29 10:49:39.896700883 +0100
+++ gcc/tree-streamer-in.c	2019-06-01 16:38:35.421667415 +0100
@@ -307,6 +307,8 @@ unpack_ts_decl_with_vis_value_fields (st
     {
       DECL_HARD_REGISTER (expr) = (unsigned) bp_unpack_value (bp, 1);
       DECL_IN_CONSTANT_POOL (expr) = (unsigned) bp_unpack_value (bp, 1);
+      DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P (expr)
+	= (unsigned) bp_unpack_value (bp, 1);
     }
 
   if (TREE_CODE (expr) == FUNCTION_DECL)
Index: gcc/tree-streamer-out.c
===================================================================
--- gcc/tree-streamer-out.c	2019-05-29 10:49:39.620701683 +0100
+++ gcc/tree-streamer-out.c	2019-06-01 16:38:35.421667415 +0100
@@ -269,6 +269,7 @@ pack_ts_decl_with_vis_value_fields (stru
       bp_pack_value (bp, DECL_HARD_REGISTER (expr), 1);
       /* DECL_IN_TEXT_SECTION is set during final asm output only. */
       bp_pack_value (bp, DECL_IN_CONSTANT_POOL (expr), 1);
+      bp_pack_value (bp, DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P (expr), 1);
     }
 
   if (TREE_CODE (expr) == FUNCTION_DECL)
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	2019-05-29 10:49:37.876706747 +0100
+++ gcc/tree.h	2019-06-01 16:38:35.421667415 +0100
@@ -2888,6 +2888,12 @@ #define DECL_DEBUG_EXPR(NODE) \
 #define SET_DECL_DEBUG_EXPR(NODE, VAL) \
   (decl_debug_expr_insert (VAR_DECL_CHECK (NODE), VAL))
 
+/* For a VAR_DECL, true if accesses to the variable can't be rewritten
+   into SSA form but if we nevertheless want to use debug stmts
+   (and later debug insns) to track its location.  */
+#define DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P(NODE) \
+  (VAR_DECL_CHECK (NODE)->decl_with_vis.track_direct_refs_for_debug_p)
+
 extern priority_type decl_init_priority_lookup (tree);
 extern priority_type decl_fini_priority_lookup (tree);
 extern void decl_init_priority_insert (tree, priority_type);
@@ -4733,6 +4739,15 @@ extern tree get_unwidened (tree, tree);
 
 extern tree get_narrower (tree, int *);
 
+/* Return true if T is a DECL for which DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P
+   applies.  */
+
+inline bool
+track_direct_refs_for_debug_p (const_tree t)
+{
+  return VAR_P (t) && DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P (t);
+}
+
 /* Return true if T is an expression that get_inner_reference handles.  */
 
 static inline bool
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	2019-05-29 10:49:39.524701962 +0100
+++ gcc/cfgexpand.c	2019-06-01 16:38:35.413667438 +0100
@@ -5429,8 +5429,15 @@ expand_debug_locations (void)
 	  val = NULL_RTX;
 	else
 	  {
-	    if (INSN_VAR_LOCATION_STATUS (insn)
-		== VAR_INIT_STATUS_UNINITIALIZED)
+	    if (INSN_VAR_LOCATION_DECL (insn) == value)
+	      {
+		if (DECL_RTL_SET_P (value))
+		  val = gen_rtx_DECL_RTL_REF (DECL_MODE (value), value);
+		else
+		  val = NULL_RTX;
+	      }
+	    else if (INSN_VAR_LOCATION_STATUS (insn)
+		     == VAR_INIT_STATUS_UNINITIALIZED)
 	      val = expand_debug_source_expr (value);
 	    /* The avoid_deep_ter_for_debug function inserts
 	       debug bind stmts after SSA_NAME definition, with the
@@ -5900,6 +5907,19 @@ expand_gimple_basic_block (basic_block b
 	      maybe_dump_rtl_for_gimple_stmt (stmt, last);
 	    }
 	}
+      if (MAY_HAVE_DEBUG_BIND_INSNS)
+	{
+	  /* If the statement is assigning to a memory variable whose
+	     location we're tracking for debug purposes, emit a note
+	     to say that the variable now lives in memory.  */
+	  tree lhs = gimple_get_lhs (stmt);
+	  if (lhs && track_direct_refs_for_debug_p (lhs))
+	    {
+	      rtx val = gen_rtx_VAR_LOCATION (DECL_MODE (lhs), lhs, (rtx) lhs,
+					      VAR_INIT_STATUS_INITIALIZED);
+	      emit_debug_insn (val);
+	    }
+	}
     }
 
   currently_expanding_gimple_stmt = NULL;
Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c	2019-05-31 17:26:54.647201231 +0100
+++ gcc/gimplify.c	2019-06-01 16:38:35.417667427 +0100
@@ -1295,6 +1295,21 @@ asan_poison_variables (hash_set<tree> *v
     }
 }
 
+/* Return true if variable VAR is local to its BIND_EXPR and might need
+   to live in memory.  */
+
+static bool
+scoped_possible_mem_var_p (tree var)
+{
+  return (!DECL_HARD_REGISTER (var)
+	  && !TREE_THIS_VOLATILE (var)
+	  && !DECL_HAS_VALUE_EXPR_P (var)
+	  /* Only care about variables that have to be in memory.  Others
+	     will be rewritten into SSA names, hence moved to the
+	     top-level.  */
+	  && !is_gimple_reg (var));
+}
+
 /* Gimplify a BIND_EXPR.  Just voidify and recurse.  */
 
 static enum gimplify_status
@@ -1313,6 +1328,7 @@ gimplify_bind_expr (tree *expr_p, gimple
   tree temp = voidify_wrapper_expr (bind_expr, NULL);
 
   /* Mark variables seen in this bind expr.  */
+  body = NULL;
   for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
     {
       if (VAR_P (t))
@@ -1348,6 +1364,18 @@ gimplify_bind_expr (tree *expr_p, gimple
 	  && (VAR_P (t) && !DECL_HARD_REGISTER (t))
 	  && !needs_to_live_in_memory (t))
 	DECL_GIMPLE_REG_P (t) = 1;
+
+      if (MAY_HAVE_DEBUG_BIND_INSNS
+	  && VAR_P (t)
+	  && !is_global_var (t)
+	  && DECL_CONTEXT (t) == current_function_decl
+	  && scoped_possible_mem_var_p (t)
+	  && target_for_debug_bind (t) == t)
+	{
+	  gdebug *sb = gimple_build_debug_source_bind (t, t, NULL);
+	  gimple_seq_add_stmt (&body, sb);
+	  DECL_TRACK_DIRECT_REFS_FOR_DEBUG_P (t) = 1;
+	}
     }
 
   bind_stmt = gimple_build_bind (BIND_EXPR_VARS (bind_expr), NULL,
@@ -1358,7 +1386,6 @@ gimplify_bind_expr (tree *expr_p, gimple
   gimplify_ctxp->save_stack = false;
 
   /* Gimplify the body into the GIMPLE_BIND tuple's body.  */
-  body = NULL;
   gimplify_stmt (&BIND_EXPR_BODY (bind_expr), &body);
   gimple_bind_set_body (bind_stmt, body);
 
@@ -1401,13 +1428,7 @@ gimplify_bind_expr (tree *expr_p, gimple
 	  && !is_global_var (t)
 	  && DECL_CONTEXT (t) == current_function_decl)
 	{
-	  if (!DECL_HARD_REGISTER (t)
-	      && !TREE_THIS_VOLATILE (t)
-	      && !DECL_HAS_VALUE_EXPR_P (t)
-	      /* Only care for variables that have to be in memory.  Others
-		 will be rewritten into SSA names, hence moved to the
-		 top-level.  */
-	      && !is_gimple_reg (t)
+	  if (scoped_possible_mem_var_p (t)
 	      && flag_stack_reuse != SR_NONE)
 	    {
 	      tree clobber = build_clobber (TREE_TYPE (t));
Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c	2019-04-04 08:34:52.221937259 +0100
+++ gcc/gimple-low.c	2019-06-01 16:38:35.417667427 +0100
@@ -309,10 +309,12 @@ lower_stmt (gimple_stmt_iterator *gsi, s
       break;
 
     case GIMPLE_DEBUG:
-      gcc_checking_assert (cfun->debug_nonbind_markers);
-      /* We can't possibly have debug bind stmts before lowering, we
-	 first emit them when entering SSA.  */
-      gcc_checking_assert (gimple_debug_nonbind_marker_p (stmt));
+      /* The only debug bind stmts we create before lowering are
+	 source binds of the form "x s=> x", to mark the start of
+	 a variable's lifetime.  */
+      gcc_checking_assert ((cfun->debug_nonbind_markers
+			    && gimple_debug_nonbind_marker_p (stmt))
+			   || gimple_debug_source_bind_p (stmt));
       /* Propagate fallthruness.  */
       /* If the function (e.g. from PCH) had debug stmts, but they're
 	 disabled for this compilation, remove them.  */
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c	2019-05-29 10:49:39.900700870 +0100
+++ gcc/tree-cfg.c	2019-06-01 16:38:35.421667415 +0100
@@ -593,8 +593,8 @@ make_blocks_1 (gimple_seq seq, basic_blo
 static void
 make_blocks (gimple_seq seq)
 {
-  /* Look for debug markers right before labels, and move the debug
-     stmts after the labels.  Accepting labels among debug markers
+  /* Look for debug stmts right before labels, and move the debug
+     stmts after the labels.  Accepting labels among debug stmts
      adds no value, just complexity; if we wanted to annotate labels
      with view numbers (so sequencing among markers would matter) or
      somesuch, we're probably better off still moving the labels, but
@@ -635,7 +635,11 @@ make_blocks (gimple_seq seq)
 	  /* Move the debug stmt at I after LABEL.  */
 	  if (is_gimple_debug (stmt))
 	    {
-	      gcc_assert (gimple_debug_nonbind_marker_p (stmt));
+	      /* Handle both debug markers and source binds of the form
+		 VAR s=> VAR (which denote the beginning of VAR's
+		 lifetime).  */
+	      gcc_assert (gimple_debug_nonbind_marker_p (stmt)
+			  || gimple_debug_source_bind_p (stmt));
 	      /* As STMT is removed, I advances to the stmt after
 		 STMT, so the gsi_prev in the for "increment"
 		 expression gets us to the stmt we're to visit after
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	2019-05-29 10:49:39.892700893 +0100
+++ gcc/tree-ssa.c	2019-06-01 16:38:35.421667415 +0100
@@ -293,9 +293,13 @@ find_released_ssa_name (tree *tp, int *w
   return NULL_TREE;
 }
 
-/* Insert a DEBUG BIND stmt before the DEF of VAR if VAR is referenced
-   by other DEBUG stmts, and replace uses of the DEF with the
-   newly-created debug temp.  */
+/* Insert a DEBUG BIND stmt before the DEF of VAR in cases where either:
+
+   - VAR is a decl that satisfies track_direct_refs_for_debug_p
+   - VAR is an SSA_NAME that is referenced by other DEBUG stmts
+
+   In the latter case, replace uses of the DEF with the newly-created
+   debug temp.  */
 
 void
 insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
@@ -310,35 +314,38 @@ insert_debug_temp_for_var_def (gimple_st
   if (!MAY_HAVE_DEBUG_BIND_STMTS)
     return;
 
-  /* If this name has already been registered for replacement, do nothing
-     as anything that uses this name isn't in SSA form.  */
-  if (name_registered_for_update_p (var))
-    return;
-
-  /* Check whether there are debug stmts that reference this variable and,
-     if there are, decide whether we should use a debug temp.  */
-  FOR_EACH_IMM_USE_FAST (use_p, imm_iter, var)
+  if (TREE_CODE (var) == SSA_NAME)
     {
-      stmt = USE_STMT (use_p);
+      /* If this name has already been registered for replacement, do nothing
+	 as anything that uses this name isn't in SSA form.  */
+      if (name_registered_for_update_p (var))
+	return;
+
+      /* Check whether there are debug stmts that reference this variable and,
+	 if there are, decide whether we should use a debug temp.  */
+      FOR_EACH_IMM_USE_FAST (use_p, imm_iter, var)
+	{
+	  stmt = USE_STMT (use_p);
 
-      if (!gimple_debug_bind_p (stmt))
-	continue;
+	  if (!gimple_debug_bind_p (stmt))
+	    continue;
 
-      if (usecount++)
-	break;
+	  if (usecount++)
+	    break;
 
-      if (gimple_debug_bind_get_value (stmt) != var)
-	{
-	  /* Count this as an additional use, so as to make sure we
-	     use a temp unless VAR's definition has a SINGLE_RHS that
-	     can be shared.  */
-	  usecount++;
-	  break;
+	  if (gimple_debug_bind_get_value (stmt) != var)
+	    {
+	      /* Count this as an additional use, so as to make sure we
+		 use a temp unless VAR's definition has a SINGLE_RHS that
+		 can be shared.  */
+	      usecount++;
+	      break;
+	    }
 	}
-    }
 
-  if (!usecount)
-    return;
+      if (!usecount)
+	return;
+    }
 
   if (gsi)
     def_stmt = gsi_stmt (*gsi);
@@ -459,32 +466,38 @@ insert_debug_temp_for_var_def (gimple_st
 	}
     }
 
-  FOR_EACH_IMM_USE_STMT (stmt, imm_iter, var)
-    {
-      if (!gimple_debug_bind_p (stmt))
-	continue;
-
-      if (value)
-	{
-	  FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
-	    /* unshare_expr is not needed here.  vexpr is either a
-	       SINGLE_RHS, that can be safely shared, some other RHS
-	       that was unshared when we found it had a single debug
-	       use, or a DEBUG_EXPR_DECL, that can be safely
-	       shared.  */
-	    SET_USE (use_p, unshare_expr (value));
-	  /* If we didn't replace uses with a debug decl fold the
-	     resulting expression.  Otherwise we end up with invalid IL.  */
-	  if (TREE_CODE (value) != DEBUG_EXPR_DECL)
-	    {
-	      gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
-	      fold_stmt_inplace (&gsi);
-	    }
-	}
-      else
-	gimple_debug_bind_reset_value (stmt);
+  if (TREE_CODE (var) == SSA_NAME)
+    FOR_EACH_IMM_USE_STMT (stmt, imm_iter, var)
+      {
+	if (!gimple_debug_bind_p (stmt))
+	  continue;
+
+	if (value)
+	  {
+	    FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
+	      /* unshare_expr is not needed here.  vexpr is either a
+		 SINGLE_RHS, that can be safely shared, some other RHS
+		 that was unshared when we found it had a single debug
+		 use, or a DEBUG_EXPR_DECL, that can be safely
+		 shared.  */
+	      SET_USE (use_p, unshare_expr (value));
+	    /* If we didn't replace uses with a debug decl fold the
+	       resulting expression.  Otherwise we end up with invalid IL.  */
+	    if (TREE_CODE (value) != DEBUG_EXPR_DECL)
+	      {
+		gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+		fold_stmt_inplace (&gsi);
+	      }
+	  }
+	else
+	  gimple_debug_bind_reset_value (stmt);
 
-      update_stmt (stmt);
+	update_stmt (stmt);
+      }
+  else
+    {
+      gdebug *bind = gimple_build_debug_bind (var, value, def_stmt);
+      gsi_insert_before (gsi, bind, GSI_SAME_STMT);
     }
 }
 
@@ -514,6 +527,10 @@ insert_debug_temps_for_defs (gimple_stmt
 
       insert_debug_temp_for_var_def (gsi, var);
     }
+
+  tree lhs = gimple_get_lhs (stmt);
+  if (lhs && track_direct_refs_for_debug_p (lhs))
+    insert_debug_temp_for_var_def (gsi, lhs);
 }
 
 /* Reset all debug stmts that use SSA_NAME(s) defined in STMT.  */
Index: gcc/testsuite/gcc.dg/guality/addr-taken-1.c
===================================================================
--- /dev/null	2019-03-08 11:40:14.606883727 +0000
+++ gcc/testsuite/gcc.dg/guality/addr-taken-1.c	2019-06-01 16:38:35.417667427 +0100
@@ -0,0 +1,36 @@
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+void __attribute__((noipa))
+get_start (int *x)
+{
+  *x = 42;
+}
+
+void __attribute__((noipa))
+consume (int *x)
+{
+  *x += 1;
+}
+
+void __attribute__((noipa))
+test (int *x, int i1, int i2, int i3, int i4)
+{
+  int base;
+  get_start (&base);
+  x[i1] = base; /* { dg-final { gdb-test . "base" "42" } } */
+  base = 100;
+  x[i2] = base; /* { dg-final { gdb-test . "base" "100" } } */
+  base += 1;
+  x[i3] = base; /* { dg-final { gdb-test . "base" "101" } } */
+  base += 1;
+  consume (&base); /* { dg-final { gdb-test . "base" "102" } } */
+  x[i4] = base; /* { dg-final { gdb-test . "base" "103" } } */
+}
+
+int
+main (void)
+{
+  int x[4];
+  test (x, 0, 1, 2, 3);
+}
Index: gcc/testsuite/gcc.dg/guality/addr-taken-2.c
===================================================================
--- /dev/null	2019-03-08 11:40:14.606883727 +0000
+++ gcc/testsuite/gcc.dg/guality/addr-taken-2.c	2019-06-01 16:38:35.417667427 +0100
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+void __attribute__((noipa))
+get_start (int *x)
+{
+  *x = 42;
+}
+
+void __attribute__((noipa))
+consume (int *x)
+{
+  *x += 1;
+}
+
+void __attribute__((noipa))
+test (int *x, int i1, int i2, int i3)
+{
+  int base;
+  get_start (&base);
+  x[i1] = base; /* { dg-final { gdb-test . "base" "42" } } */
+  base += 1;
+  x[i2] = base; /* { dg-final { gdb-test . "base" "43" } } */
+  base += 1;
+  consume (&base); /* { dg-final { gdb-test . "base" "44" } } */
+  x[i3] = base; /* { dg-final { gdb-test . "base" "45" } } */
+}
+
+int
+main (void)
+{
+  int x[3];
+  test (x, 0, 1, 2);
+}

[3/3] Fix debug info for LSM

[Richard Sandiford]

This patch makes the into-SSA code handle LSM temporary variables
as though they had been assignments to the original variable.

In the end, the easiest place to record the link seemed to be
DECL_ABSTRACT_ORIGIN.  These sorts of nameless temporaries shouldn't
otherwise have debug info and so shouldn't be using DECL_ABSTRACT_ORIGIN
for anything else.


2019-06-01  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* gimple.h (gimple_assign_load_decl_p): New function.
	* tree-ssa.h (inherit_target_for_debug_bind): Declare.
	* tree-ssa.c (inherit_target_for_debug_bind): New function.
	(target_for_debug_bind): Honor calls to inherit_target_for_debug_bind.
	* tree-into-ssa.c (maybe_register_def): Don't emit "VAR => DEF"
	for "DEF = VAR".
	* tree-ssa-loop-im.c: Include tree-ssa.h.
	(execute_sm_if_changed): Explain why we don't insert any debug
	statements here.
	(execute_sm): Call inherit_target_for_debug_bind on the temporary
	variable.

gcc/testsuite/
	* lib/gcc-gdb-test.exp (gdb-test): Add a syntax for specifying
	the Nth hit of a breakpoint.
	* gcc.dg/guality/loop-2.c: New test.
	* gcc.dg/guality/loop-3.c: Likewise.
	* gcc.dg/guality/loop-4.c: Likewise.

Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h	2019-06-01 16:52:38.000000000 +0100
+++ gcc/gimple.h	2019-06-01 16:52:39.075271587 +0100
@@ -2782,6 +2782,18 @@ gimple_assign_load_p (const gimple *gs)
 }
 
 
+/* Return true if GS is an assignment that loads DECL into its lhs.  */
+
+static inline bool
+gimple_assign_load_decl_p (const gimple *gs, tree decl)
+{
+  const gassign *assign = dyn_cast <const gassign *> (gs);
+  return (assign
+	  && gimple_assign_single_p (assign)
+	  && get_base_address (gimple_assign_rhs1 (assign)) == decl);
+}
+
+
 /* Return true if S is a type-cast assignment.  */
 
 static inline bool
Index: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h	2019-06-01 16:52:38.000000000 +0100
+++ gcc/tree-ssa.h	2019-06-01 16:52:39.079271578 +0100
@@ -39,6 +39,7 @@ extern void redirect_edge_var_map_empty
 extern edge ssa_redirect_edge (edge, basic_block);
 extern void flush_pending_stmts (edge);
 extern void gimple_replace_ssa_lhs (gimple *, tree);
+extern void inherit_target_for_debug_bind (tree, tree);
 extern tree target_for_debug_bind (tree);
 extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
 extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	2019-06-01 16:52:38.000000000 +0100
+++ gcc/tree-ssa.c	2019-06-01 16:52:39.079271578 +0100
@@ -232,6 +232,25 @@ gimple_replace_ssa_lhs (gimple *stmt, tr
   gimple_set_lhs (stmt, nlhs);
 }
 
+/* Record that assignments to new temporary variable VAR should be treated
+   for debug purposes like an assignment to ORIGIN.  More specifically,
+   record that the value of target_for_debug_bind (VAR) should track the
+   value of target_for_debug_bind (ORIGIN).
+
+   This can be useful when replacing all references to ORIGIN with VAR
+   in a particular region of code.  */
+
+void
+inherit_target_for_debug_bind (tree var, tree origin)
+{
+  gcc_assert (VAR_P (var)
+	      && DECL_IGNORED_P (var)
+	      && DECL_ARTIFICIAL (var)
+	      && DECL_NAMELESS (var)
+	      && !DECL_ABSTRACT_ORIGIN (var));
+  if (target_for_debug_bind (origin))
+    DECL_ABSTRACT_ORIGIN (var) = origin;
+}
 
 /* Given a tree for an expression for which we might want to emit
    locations or values in debug information (generally a variable, but
@@ -252,6 +271,12 @@ target_for_debug_bind (tree var)
 	return NULL_TREE;
     }
 
+  /* Honor choices made through inherit_target_for_debug_bind.  */
+  if (VAR_P (var)
+      && DECL_IGNORED_P (var)
+      && !DECL_IGNORED_P (DECL_ORIGIN (var)))
+    var = DECL_ORIGIN (var);
+
   if ((!VAR_P (var) || VAR_DECL_IS_VIRTUAL_OPERAND (var))
       && TREE_CODE (var) != PARM_DECL)
     return NULL_TREE;
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c	2019-06-01 16:52:38.000000000 +0100
+++ gcc/tree-into-ssa.c	2019-06-01 16:52:39.079271578 +0100
@@ -1916,7 +1916,12 @@ maybe_register_def (def_operand_p def_p,
 	  SET_DEF (def_p, def);
 
 	  tree tracked_var = target_for_debug_bind (sym);
-	  if (tracked_var)
+	  /* Don't emit "VAR => DEF" for "DEF = VAR".  Although not
+	     semantically wrong, it leads to more resets when the load
+	     from VAR is (or might become) partly speculative.  */
+	  if (tracked_var
+	      && !(track_direct_refs_for_debug_p (tracked_var)
+		   && gimple_assign_load_decl_p (stmt, tracked_var)))
 	    {
 	      gimple *note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
Index: gcc/tree-ssa-loop-im.c
===================================================================
--- gcc/tree-ssa-loop-im.c	2019-06-01 16:52:38.000000000 +0100
+++ gcc/tree-ssa-loop-im.c	2019-06-01 16:52:39.079271578 +0100
@@ -48,6 +48,7 @@ Free Software Foundation; either version
 #include "alias.h"
 #include "builtins.h"
 #include "tree-dfa.h"
+#include "tree-ssa.h"
 
 /* TODO:  Support for predicated code motion.  I.e.
 
@@ -1998,6 +1999,24 @@ execute_sm_if_changed (edge ex, tree mem
     orig_ex->aux = (void *) p;
   }
 
+  /* ??? As things stand, the value of MEM on entry to the join block
+     can be found at MEM's DECL_RTL.  It would therefore be accurate
+     to emit:
+
+         # DEBUG MEM s=> MEM
+
+     on entry to the block.  However, there's no guarantee that later
+     optimizations will keep things that way, and they can't reasonably
+     be expected to find and correct the statement.
+
+     E.g. if the store in THEN_BB is later deleted as dead, the debug
+     statement above would reestablish the connection between MEM and
+     its DECL_RTL even though the two are no longer the same.
+
+     We therefore leave things so that the debug location on entry
+     to the join block comes from MEM's DECL_RTL if the flag is set
+     and TMP_VAR otherwise.  */
+
   if (!loop_has_only_one_exit)
     for (gphi_iterator gpi = gsi_start_phis (old_dest);
 	 !gsi_end_p (gpi); gsi_next (&gpi))
@@ -2082,6 +2101,7 @@ execute_sm (struct loop *loop, vec<edge>
 
   tmp_var = create_tmp_reg (TREE_TYPE (ref->mem.ref),
 			    get_lsm_tmp_name (ref->mem.ref, ~0));
+  inherit_target_for_debug_bind (tmp_var, ref->mem.ref);
 
   fmt_data.loop = loop;
   fmt_data.orig_loop = loop;
Index: gcc/testsuite/lib/gcc-gdb-test.exp
===================================================================
--- gcc/testsuite/lib/gcc-gdb-test.exp	2019-06-01 16:52:38.000000000 +0100
+++ gcc/testsuite/lib/gcc-gdb-test.exp	2019-06-01 16:52:39.075271587 +0100
@@ -18,6 +18,7 @@
 # Call pass if variable has the desired value, otherwise fail.
 #
 # Argument 0 is the line number on which to put a breakpoint
+#   It can be suffixed by "*N" to test the Nth hit of the breakpoint.
 # Argument 1 is the name of the variable to be checked
 #   possibly prefixed with type: to get the type of the variable
 #   instead of the value of the variable (the default).
@@ -54,12 +55,18 @@ proc gdb-test { useline args } {
 	set var $arg1
     }
 
-    set line [lindex $args 0]
+    set line_pieces [split [lindex $args 0] "*"]
+    set line [lindex $line_pieces 0]
     if { [string range $line 0 0] == "@" } {
 	set line [string range $line 1 end]
     } else {
 	set line [get-absolute-line $useline $line]
     }
+    if { [llength $line_pieces] > 1 } {
+	set count [lindex $line_pieces 1]
+    } else {
+	set count 1
+    }
 
     set gdb_name $::env(GUALITY_GDB_NAME)
     set testname "$testcase line $line [lindex $args 1] == [lindex $args 2]"
@@ -69,6 +76,9 @@ proc gdb-test { useline args } {
     set fd [open $cmd_file "w"]
     puts $fd "break $line"
     puts $fd "run"
+    if { $count > 1 } {
+	puts $fd "continue [expr { $count - 1 }]"
+    }
     puts $fd "$command $var"
     if { $command == "print" } {
 	# For values, let gdb interpret them by printing them.
Index: gcc/testsuite/gcc.dg/guality/loop-2.c
===================================================================
--- /dev/null	2019-03-08 11:40:14.606883727 +0000
+++ gcc/testsuite/gcc.dg/guality/loop-2.c	2019-06-01 16:52:39.075271587 +0100
@@ -0,0 +1,41 @@
+/* { dg-do run } */
+/* Works without the --param except at -Os.  */
+/* { dg-options "-fno-tree-vectorize --param allow-store-data-races=1 -g" } */
+
+void __attribute__((noipa))
+get_start (int *x)
+{
+  *x = 42;
+}
+
+void __attribute__((noipa))
+consume (int *x)
+{
+  *x += 1;
+}
+
+void __attribute__((noipa))
+test (int *x, int *y, int n)
+{
+  int base;
+  get_start (&base);
+  base += 1; /* { dg-final { gdb-test . "base" "42" } } */
+  get_start (&base); /* { dg-final { gdb-test . "base" "43" } } */
+  for (int i = 0; i < n; ++i)
+    {
+      x[i] = base; /* { dg-final { gdb-test .*10 "base" "51" } } */
+      base += y[i];
+    }
+  y[0] = base; /* { dg-final { gdb-test . "base" "142" } } */
+  consume (&base); /* { dg-final { gdb-test . "base" "142" } } */
+  y[1] = 1; /* { dg-final { gdb-test . "base" "143" } } */
+}
+
+int
+main (void)
+{
+  int x[100], y[100];
+  for (int i = 0; i < 100; ++i)
+    y[i] = 1;
+  test (x, y, 100);
+}
Index: gcc/testsuite/gcc.dg/guality/loop-3.c
===================================================================
--- /dev/null	2019-03-08 11:40:14.606883727 +0000
+++ gcc/testsuite/gcc.dg/guality/loop-3.c	2019-06-01 16:52:39.075271587 +0100
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options "-fno-tree-vectorize -fno-unroll-loops -g" } */
+
+void __attribute__((noipa))
+get_start (int *x)
+{
+  *x = 42;
+}
+
+void __attribute__((noipa))
+consume (int *x)
+{
+  *x += 1;
+}
+
+void __attribute__((noipa))
+test (int *x, int *y)
+{
+  int base;
+  get_start (&base);
+  base += 1; /* { dg-final { gdb-test . "base" "42" } } */
+  get_start (&base); /* { dg-final { gdb-test . "base" "43" } } */
+  for (int i = 0; i < 100; ++i)
+    {
+      x[i] = base; /* { dg-final { gdb-test .*10 "base" "51" } } */
+      base += y[i];
+    }
+  y[0] = base; /* { dg-final { gdb-test . "base" "142" } } */
+}
+
+int
+main (void)
+{
+  int x[100], y[100];
+  for (int i = 0; i < 100; ++i)
+    y[i] = 1;
+  test (x, y);
+}
Index: gcc/testsuite/gcc.dg/guality/loop-4.c
===================================================================
--- /dev/null	2019-03-08 11:40:14.606883727 +0000
+++ gcc/testsuite/gcc.dg/guality/loop-4.c	2019-06-01 16:52:39.075271587 +0100
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-fno-tree-vectorize -fno-unroll-loops -g" } */
+
+void __attribute__((noipa))
+get_start (int *x)
+{
+  *x = 42;
+}
+
+void __attribute__((noipa))
+test (int *x, int *y)
+{
+  int base;
+  get_start (&base);
+  base += 1; /* { dg-final { gdb-test . "base" "42" } } */
+  get_start (&base);
+  for (int i = 0; i < 100; ++i)
+    {
+      x[i] = base; /* { dg-final { gdb-test .*10 "base" "51" } } */
+      base += y[i];
+    }
+  y[0] = base; /* { dg-final { gdb-test . "base" "142" } } */
+  base += 1;
+  y[1] = base; /* { dg-final { gdb-test . "base" "143" } } */
+}
+
+int
+main (void)
+{
+  int x[100], y[100];
+  for (int i = 0; i < 100; ++i)
+    y[i] = 1;
+  test (x, y);
+}

Re: [0/3] Improve debug info for addressable vars

[Richard Biener]

On Sat, Jun 1, 2019 at 5:49 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Taking the address of a variable stops us doing var-tracking on it,
> so that we just use the DECL_RTL instead.  This can easily cause wrong
> debug info for regions of code that would have had correct debug info
> if the variable weren't addressable.  E.g.:
>
> {
>   int base;
>   get_start (&base);
>   x[i1] = base;
>   base += 1; // No need to store this
>   x[i2] = base; // ...so the debug info for "base" is wrong here
> }
>
> or (the motivating example):
>
> {
>   int base;
>   get_start (&base);
>   for (int i = 0; i < n; ++i)
>     {
>       x[i] = base;
>       base += y[i]; // Can apply LSM here, so the debug info for "base"
>                     // in the loop is wrong
>     }
>   consume (&base);
> }
>
> This patch series lets us use the DECL_RTL location for some parts of a
> variable's lifetime and debug-bind locations for other parts:
>
> 1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL.  The binding
>    holds until overridden.
>
> 2) RTL does the same thing using:
>
>      (var_location VAR (decl_rtl_ref VAR))
>
>    where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
>    by reference rather than by value.
>
>    We can't just use "(var_location VAR (mem X))" for this, because
>    that would bind VAR to the value that (mem X) has at that exact point.
>    VAR would therefore get reset by any possible change to (mem X),
>    whereas here we want it to track (possibly indirect) updates instead.
>
> 3) The gimplifier decides which variables should get the new treatment
>    and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
>    Clobbers continue to mark the end of VAR's lifetime.
>
> 4) Stores to VAR implicitly reestablish the link between VAR and its
>    DECL_RTL.  This is simpler (and IMO more robust) than inserting an
>    explicit "VAR s=> VAR" at every write.
>
> 5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
>    falling back to a "VAR => NULL" reset if that fails.
>
> Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
> and patch 3 uses it for LSM.

So I wonder how it handles

void __attribute__((noinline)) foo(int *p) { *p = 42; }
int x;
int main()
{
  int base = 1;
  foo (&base);
  base = 2;
  *(x ? &x : &base) = 1; // (*)
  return 0;
}

here we DSE the base = 2 store leaving a

# DEBUG base = 2

stmt?  But there's an indirect store that also stores
to base - what will the debug info say at/after (*)?  Will
it claim that base is 2?  At least I do not see that
the connection with bases DECL_RTL is re-established?
There's a clobber of base before return 0 so you eventually
have to add some dummy stmt you can print base after
the indirect store.

That said, doesn't "aliasing" create another source of wrong-debug
with your approach that might be even worse?

Otherwise the patches look reasonable.

Richard.

> Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
> OK to install?
>
> Richard

Re: [0/3] Improve debug info for addressable vars

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Sat, Jun 1, 2019 at 5:49 PM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Taking the address of a variable stops us doing var-tracking on it,
>> so that we just use the DECL_RTL instead.  This can easily cause wrong
>> debug info for regions of code that would have had correct debug info
>> if the variable weren't addressable.  E.g.:
>>
>> {
>>   int base;
>>   get_start (&base);
>>   x[i1] = base;
>>   base += 1; // No need to store this
>>   x[i2] = base; // ...so the debug info for "base" is wrong here
>> }
>>
>> or (the motivating example):
>>
>> {
>>   int base;
>>   get_start (&base);
>>   for (int i = 0; i < n; ++i)
>>     {
>>       x[i] = base;
>>       base += y[i]; // Can apply LSM here, so the debug info for "base"
>>                     // in the loop is wrong
>>     }
>>   consume (&base);
>> }
>>
>> This patch series lets us use the DECL_RTL location for some parts of a
>> variable's lifetime and debug-bind locations for other parts:
>>
>> 1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL.  The binding
>>    holds until overridden.
>>
>> 2) RTL does the same thing using:
>>
>>      (var_location VAR (decl_rtl_ref VAR))
>>
>>    where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
>>    by reference rather than by value.
>>
>>    We can't just use "(var_location VAR (mem X))" for this, because
>>    that would bind VAR to the value that (mem X) has at that exact point.
>>    VAR would therefore get reset by any possible change to (mem X),
>>    whereas here we want it to track (possibly indirect) updates instead.
>>
>> 3) The gimplifier decides which variables should get the new treatment
>>    and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
>>    Clobbers continue to mark the end of VAR's lifetime.
>>
>> 4) Stores to VAR implicitly reestablish the link between VAR and its
>>    DECL_RTL.  This is simpler (and IMO more robust) than inserting an
>>    explicit "VAR s=> VAR" at every write.
>>
>> 5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
>>    falling back to a "VAR => NULL" reset if that fails.
>>
>> Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
>> and patch 3 uses it for LSM.
>
> So I wonder how it handles
>
> void __attribute__((noinline)) foo(int *p) { *p = 42; }
> int x;
> int main()
> {
>   int base = 1;
>   foo (&base);
>   base = 2;
>   *(x ? &x : &base) = 1; // (*)
>   return 0;
> }
>
> here we DSE the base = 2 store leaving a
>
> # DEBUG base = 2
>
> stmt?  But there's an indirect store that also stores
> to base - what will the debug info say at/after (*)?  Will
> it claim that base is 2?  At least I do not see that
> the connection with bases DECL_RTL is re-established?

Yeah, true.

> There's a clobber of base before return 0 so you eventually
> have to add some dummy stmt you can print base after
> the indirect store.
>
> That said, doesn't "aliasing" create another source of wrong-debug
> with your approach that might be even worse?

Not sure about even worse, but maybe different.  In the example above
the patches fix the debug info after "base = 2" but break it after the
following statement.

But there's no real need for the compiler to store to base in (*) either.
We could end up with "if (...) x = 1;" instead.  So AFAICT there's no
guarantee that we'll get correct debug info at the return statement even
as things stand.

For memory variables, I think we're always at the mercy of dead stores
being optimised away, and the patch isn't trying to fix that.  Since
both writes to base are dead in the above, I don't think we can guarantee
correct debug info without compromising optimisation for the sake of
debuggability.  (FWIW, I have a WIP patch to add an option for that,
hope to post an RFC soon.)

I can't think of a case in which the patches introduce wrong debug
info for code that isn't dead.

Thanks,
Richard

>
> Otherwise the patches look reasonable.
>
> Richard.
>
>> Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
>> OK to install?
>>
>> Richard

Re: [0/3] Improve debug info for addressable vars

[Richard Biener]

On Wed, Jun 5, 2019 at 4:30 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On Sat, Jun 1, 2019 at 5:49 PM Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Taking the address of a variable stops us doing var-tracking on it,
> >> so that we just use the DECL_RTL instead.  This can easily cause wrong
> >> debug info for regions of code that would have had correct debug info
> >> if the variable weren't addressable.  E.g.:
> >>
> >> {
> >>   int base;
> >>   get_start (&base);
> >>   x[i1] = base;
> >>   base += 1; // No need to store this
> >>   x[i2] = base; // ...so the debug info for "base" is wrong here
> >> }
> >>
> >> or (the motivating example):
> >>
> >> {
> >>   int base;
> >>   get_start (&base);
> >>   for (int i = 0; i < n; ++i)
> >>     {
> >>       x[i] = base;
> >>       base += y[i]; // Can apply LSM here, so the debug info for "base"
> >>                     // in the loop is wrong
> >>     }
> >>   consume (&base);
> >> }
> >>
> >> This patch series lets us use the DECL_RTL location for some parts of a
> >> variable's lifetime and debug-bind locations for other parts:
> >>
> >> 1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL.  The binding
> >>    holds until overridden.
> >>
> >> 2) RTL does the same thing using:
> >>
> >>      (var_location VAR (decl_rtl_ref VAR))
> >>
> >>    where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
> >>    by reference rather than by value.
> >>
> >>    We can't just use "(var_location VAR (mem X))" for this, because
> >>    that would bind VAR to the value that (mem X) has at that exact point.
> >>    VAR would therefore get reset by any possible change to (mem X),
> >>    whereas here we want it to track (possibly indirect) updates instead.
> >>
> >> 3) The gimplifier decides which variables should get the new treatment
> >>    and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
> >>    Clobbers continue to mark the end of VAR's lifetime.
> >>
> >> 4) Stores to VAR implicitly reestablish the link between VAR and its
> >>    DECL_RTL.  This is simpler (and IMO more robust) than inserting an
> >>    explicit "VAR s=> VAR" at every write.
> >>
> >> 5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
> >>    falling back to a "VAR => NULL" reset if that fails.
> >>
> >> Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
> >> and patch 3 uses it for LSM.
> >
> > So I wonder how it handles
> >
> > void __attribute__((noinline)) foo(int *p) { *p = 42; }
> > int x;
> > int main()
> > {
> >   int base = 1;
> >   foo (&base);
> >   base = 2;
> >   *(x ? &x : &base) = 1; // (*)
> >   return 0;
> > }
> >
> > here we DSE the base = 2 store leaving a
> >
> > # DEBUG base = 2
> >
> > stmt?  But there's an indirect store that also stores
> > to base - what will the debug info say at/after (*)?  Will
> > it claim that base is 2?  At least I do not see that
> > the connection with bases DECL_RTL is re-established?
>
> Yeah, true.
>
> > There's a clobber of base before return 0 so you eventually
> > have to add some dummy stmt you can print base after
> > the indirect store.
> >
> > That said, doesn't "aliasing" create another source of wrong-debug
> > with your approach that might be even worse?
>
> Not sure about even worse, but maybe different.  In the example above
> the patches fix the debug info after "base = 2" but break it after the
> following statement.
>
> But there's no real need for the compiler to store to base in (*) either.

Indeed partial dead code elim code sink the store into both arms and then
remove the store in one of them.

> We could end up with "if (...) x = 1;" instead.  So AFAICT there's no
> guarantee that we'll get correct debug info at the return statement even
> as things stand.
>
> For memory variables, I think we're always at the mercy of dead stores
> being optimised away, and the patch isn't trying to fix that.

Hmm, but you _do_ insert the debug stmts when we remove stores...

>  Since
> both writes to base are dead in the above, I don't think we can guarantee
> correct debug info without compromising optimisation for the sake of
> debuggability.  (FWIW, I have a WIP patch to add an option for that,
> hope to post an RFC soon.)
>
> I can't think of a case in which the patches introduce wrong debug
> info for code that isn't dead.

All the recent slew of wrong-debug bugs were exactly for dead code...
I don't think that the difference of dead store vs dead SSA assignment
is different from the user perception.  Now - I think that var-tracking
could fix things up here once it sees aliasing stores.  The important
difference to before is that now it suddenly deals with variables that
are aliased while before that could not happen (besides spills/reloads).
Can you quickly see what it would take to take this into account?
For my testcase the debugger should say "optimized out" because
we can't tell whether the store will actuall touch 'base', so refering
to its memory location is wrong as well (since we elided the last store).

Richard.

> Thanks,
> Richard
>
> >
> > Otherwise the patches look reasonable.
> >
> > Richard.
> >
> >> Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
> >> OK to install?
> >>
> >> Richard

Re: [0/3] Improve debug info for addressable vars

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Jun 5, 2019 at 4:30 PM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Richard Biener <richard.guenther@gmail.com> writes:
>> > On Sat, Jun 1, 2019 at 5:49 PM Richard Sandiford
>> > <richard.sandiford@arm.com> wrote:
>> >>
>> >> Taking the address of a variable stops us doing var-tracking on it,
>> >> so that we just use the DECL_RTL instead.  This can easily cause wrong
>> >> debug info for regions of code that would have had correct debug info
>> >> if the variable weren't addressable.  E.g.:
>> >>
>> >> {
>> >>   int base;
>> >>   get_start (&base);
>> >>   x[i1] = base;
>> >>   base += 1; // No need to store this
>> >>   x[i2] = base; // ...so the debug info for "base" is wrong here
>> >> }
>> >>
>> >> or (the motivating example):
>> >>
>> >> {
>> >>   int base;
>> >>   get_start (&base);
>> >>   for (int i = 0; i < n; ++i)
>> >>     {
>> >>       x[i] = base;
>> >>       base += y[i]; // Can apply LSM here, so the debug info for "base"
>> >>                     // in the loop is wrong
>> >>     }
>> >>   consume (&base);
>> >> }
>> >>
>> >> This patch series lets us use the DECL_RTL location for some parts of a
>> >> variable's lifetime and debug-bind locations for other parts:
>> >>
>> >> 1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL.  The binding
>> >>    holds until overridden.
>> >>
>> >> 2) RTL does the same thing using:
>> >>
>> >>      (var_location VAR (decl_rtl_ref VAR))
>> >>
>> >>    where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
>> >>    by reference rather than by value.
>> >>
>> >>    We can't just use "(var_location VAR (mem X))" for this, because
>> >>    that would bind VAR to the value that (mem X) has at that exact point.
>> >>    VAR would therefore get reset by any possible change to (mem X),
>> >>    whereas here we want it to track (possibly indirect) updates instead.
>> >>
>> >> 3) The gimplifier decides which variables should get the new treatment
>> >>    and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
>> >>    Clobbers continue to mark the end of VAR's lifetime.
>> >>
>> >> 4) Stores to VAR implicitly reestablish the link between VAR and its
>> >>    DECL_RTL.  This is simpler (and IMO more robust) than inserting an
>> >>    explicit "VAR s=> VAR" at every write.
>> >>
>> >> 5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
>> >>    falling back to a "VAR => NULL" reset if that fails.
>> >>
>> >> Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
>> >> and patch 3 uses it for LSM.
>> >
>> > So I wonder how it handles
>> >
>> > void __attribute__((noinline)) foo(int *p) { *p = 42; }
>> > int x;
>> > int main()
>> > {
>> >   int base = 1;
>> >   foo (&base);
>> >   base = 2;
>> >   *(x ? &x : &base) = 1; // (*)
>> >   return 0;
>> > }
>> >
>> > here we DSE the base = 2 store leaving a
>> >
>> > # DEBUG base = 2
>> >
>> > stmt?  But there's an indirect store that also stores
>> > to base - what will the debug info say at/after (*)?  Will
>> > it claim that base is 2?  At least I do not see that
>> > the connection with bases DECL_RTL is re-established?
>>
>> Yeah, true.
>>
>> > There's a clobber of base before return 0 so you eventually
>> > have to add some dummy stmt you can print base after
>> > the indirect store.
>> >
>> > That said, doesn't "aliasing" create another source of wrong-debug
>> > with your approach that might be even worse?
>>
>> Not sure about even worse, but maybe different.  In the example above
>> the patches fix the debug info after "base = 2" but break it after the
>> following statement.
>>
>> But there's no real need for the compiler to store to base in (*) either.
>
> Indeed partial dead code elim code sink the store into both arms and then
> remove the store in one of them.
>
>> We could end up with "if (...) x = 1;" instead.  So AFAICT there's no
>> guarantee that we'll get correct debug info at the return statement even
>> as things stand.
>>
>> For memory variables, I think we're always at the mercy of dead stores
>> being optimised away, and the patch isn't trying to fix that.
>
> Hmm, but you _do_ insert the debug stmts when we remove stores...

Right, but that's for examples like the ones in patch 2, where we have:

   base += 1;
   ... use base ...;

and base doesn't need to be stored back to memory after the addition.
The difference is that here the user didn't write dead code (because
the result of the addition is used), whereas in your example they did.

I.e. the reason for replacing a dead "VAR = X" with "# DEBUG VAR => X"
is that we should only be deleting the store if:

(a) all later uses of "VAR" can be/have been replaced by "X" or
(b) the calculation of "X" was never needed in the first place

(a) is something we should handle as well as we can, and I think
inserting the debug statement is the right thing to do there.
(b) seems like a lost cause.  E.g. for:

{
  int base;
  ...
  if (...)
    x = &base;
  *x = 1; // [A]
  *x = 2; // [B]
}

I don't think we're ever going to be able to make the store of 1 visible
at [B] in optimised code.

IMO it wouldn't be helpful to invalidate the debug info for all local
and global state that could alias "x" at [B] simply because [A] is
being deleted.  At least, I know I'd be frustrated if everything
suddenly became "optimised out" there. :-)

Since the original program never needed to do [A] in the first place,
I think it's OK if the effect of [A] isn't visible in the debug info.

>>  Since
>> both writes to base are dead in the above, I don't think we can guarantee
>> correct debug info without compromising optimisation for the sake of
>> debuggability.  (FWIW, I have a WIP patch to add an option for that,
>> hope to post an RFC soon.)
>>
>> I can't think of a case in which the patches introduce wrong debug
>> info for code that isn't dead.
>
> All the recent slew of wrong-debug bugs were exactly for dead code...

But dead user code?  Or dead gimple statements for necessary user code?

> I don't think that the difference of dead store vs dead SSA assignment
> is different from the user perception.  Now - I think that var-tracking
> could fix things up here once it sees aliasing stores.  The important
> difference to before is that now it suddenly deals with variables that
> are aliased while before that could not happen (besides spills/reloads).
> Can you quickly see what it would take to take this into account?
> For my testcase the debugger should say "optimized out" because
> we can't tell whether the store will actuall touch 'base', so refering
> to its memory location is wrong as well (since we elided the last store).

I think this will hurt cases without dead user code though, since
var-tracking can't be as exact as the gimple optimisers (no access
to PTA for one thing).

E.g. the gimple optimisers might be able to prove that: "*x" cannot
alias "base" in:

{
  base += ...;
  ... use base ...;
  *x += 1;
  ...
}

and so could delete the store to base.  But with the above, var-tracking
could needlessly invalidate the debug statement after "*x = ..." if it
doesn't have access to the same information.

Also, I think with that approach we'd often end up being conservatively
correct and resetting the debug info at the end of the block, since it
would likely be too expensive to track global availability for each
individual memory variable.

Thanks,
Richard