Put scope blocks on a diet

Put scope blocks on a diet

[Jan Hubicka]

Hi,
this patch adds code for prunning scope blocks that (quite surprisingly) pops
up as top memory consumers in many C++ programs.  This problem is usually
introduced by inliner that for each inlined accestor produce about 3 scope
blocks.

The patch simply removes blocks that should not be needed that is:
  - no statements assigned to them
  - no live variables assigned to them
    (I belive that for blocks you can't breakpoint to, you can't also resonably
     ask about the variable)
  - leaf blocks or nonleaf blocks having precisely one subblock (so the
    block itself can be replaced by subblock)
  - I also check that abstract origin does not change.

With debugging disabled gimplifier also knows to not assign statements
to block so we can remove more of them.  This seems to be safe to do
concerning consistency in between -g and no -g (we still keep enough
block around to support some of other tests made)

We probably can come with more fine grained rules, but I am not expert
in the area so I tried to be conservative.  The patch still saves
substantial portion of memory for testcases I am interested in.

Note also the existence of unused ignore_block debughook. This debughook used
to be used by RTL code removing paired BLOCK notes with no code in them.  What
I do above is more strict than what the debug outputs test (they usually look
for nested functions living in the block).  WIth the new code handling blocks,
we are more aggressive on removing blocks in RTL land basically removing all
blocks that are having no statements (transitively) in them.

I wonder if we want to restore original function (we would probably just need
to emit the required blocks as empty blocks on the end of function or something
since info about their original position is completely lost on this point)

Bootstrapped/regtested i686-linux and run through gdb testsuite.  OK?

Honza
	* gimple-low.c (lower_stmt): When not doing debugging, don't save
	info about block.
	* tree-ssa-live.c: Include debug.h and flags.h.
	(mark_scope_block_unused): New function.
	(remove_unused_scope_block_p): New function.
	(remove_unused_locals): Remove unused blocks too.
Index: gimple-low.c
===================================================================
--- gimple-low.c	(revision 126859)
+++ gimple-low.c	(working copy)
@@ -210,7 +210,9 @@ lower_stmt (tree_stmt_iterator *tsi, str
 {
   tree stmt = tsi_stmt (*tsi);
 
-  if (EXPR_HAS_LOCATION (stmt) && data)
+  if (EXPR_HAS_LOCATION (stmt) && data
+      && (debug_info_level == DINFO_LEVEL_NORMAL
+          || debug_info_level == DINFO_LEVEL_VERBOSE))
     TREE_BLOCK (stmt) = data->block;
 
   switch (TREE_CODE (stmt))
Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c	(revision 126859)
+++ tree-ssa-live.c	(working copy)
@@ -30,6 +30,8 @@ Boston, MA 02110-1301, USA.  */
 #include "tree-dump.h"
 #include "tree-ssa-live.h"
 #include "toplev.h"
+#include "debug.h"
+#include "flags.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
@@ -405,9 +407,15 @@ mark_all_vars_used_1 (tree *tp, int *wal
 		      void *data ATTRIBUTE_UNUSED)
 {
   tree t = *tp;
+  char const c = TREE_CODE_CLASS (TREE_CODE (t));
+  tree b;
 
   if (TREE_CODE (t) == SSA_NAME)
     t = SSA_NAME_VAR (t);
+  if ((IS_EXPR_CODE_CLASS (c)
+       || IS_GIMPLE_STMT_CODE_CLASS (c))
+      && (b = TREE_BLOCK (t)) != NULL)
+    TREE_USED (b) = true;
 
   /* Ignore TREE_ORIGINAL for TARGET_MEM_REFS, as well as other
      fields that do not contain vars.  */
@@ -431,6 +439,111 @@ mark_all_vars_used_1 (tree *tp, int *wal
   return NULL;
 }
 
+/* Mark the scope block SCOPE and is subblocks unused when they can be
+   possibly eliminated if dead.  */
+
+static void
+mark_scope_block_unused (tree scope)
+{
+  tree t;
+  TREE_USED (scope) = false;
+  if (!(*debug_hooks->ignore_block) (scope))
+    TREE_USED (scope) = true;
+  for (t = BLOCK_SUBBLOCKS (scope); t ; t = BLOCK_CHAIN (t))
+    mark_scope_block_unused (t);
+}
+
+/* Look if the block is dead (by possibly elliminating it's dead subblocks)
+   and return true if so.  
+   Block is declared dead if:
+     1) No statements are associated with it.
+     2) Declares no live variables
+     3) All subblocks are dead
+	or there is precisely one subblocks and the block
+	has same abstract origin as outer block and declares
+	no variables, so it is pure wrapper.
+   When we are not outputting full debug info, we also elliminate dead variables
+   out of scope blocks to let them to be recycled by GGC and to save copying work
+   done by the inliner.
+*/
+
+static bool
+remove_unused_scope_block_p (tree scope)
+{
+  tree *t, *next;
+  bool unused = !TREE_USED (scope);
+  var_ann_t ann;
+  int nsubblocks = 0;
+
+  for (t = &BLOCK_VARS (scope); *t; t = next)
+    {
+      next = &TREE_CHAIN (*t);
+
+      /* Debug info of nested function reffers to the block of the
+	 function.  */
+      if (TREE_CODE (*t) == FUNCTION_DECL)
+	unused = false;
+
+      /* When we are outputting debug info, we usually want to output
+	 info about optimized-out variables in the scope blocks.
+	 Exception are the scope blocks not containing any instructions
+	 at all so user can't get into the scopes at first place.  */
+      else if ((ann = var_ann (*t)) != NULL
+		&& ann->used)
+	unused = false;
+
+      /* When we are not doing full debug info, we however can keep around
+	 only the used variables for cfgexpand's memory packing saving quite
+	 a lot of memory.  */
+      else if (debug_info_level != DINFO_LEVEL_NORMAL
+	       && debug_info_level != DINFO_LEVEL_VERBOSE)
+	{
+	  *t = TREE_CHAIN (*t);
+	  next = t;
+	}
+    }
+
+  for (t = &BLOCK_SUBBLOCKS (scope); *t ;)
+    if (remove_unused_scope_block_p (*t))
+      {
+	if (BLOCK_SUBBLOCKS (*t))
+	  {
+	    tree next = BLOCK_CHAIN (*t);
+	    tree supercontext = BLOCK_SUPERCONTEXT (*t);
+	    *t = BLOCK_SUBBLOCKS (*t);
+	    gcc_assert (!BLOCK_CHAIN (*t));
+	    BLOCK_CHAIN (*t) = next;
+	    BLOCK_SUPERCONTEXT (*t) = supercontext;
+	    t = &BLOCK_CHAIN (*t);
+	    nsubblocks ++;
+	  }
+	else
+          *t = BLOCK_CHAIN (*t);
+      }
+    else
+      {
+        t = &BLOCK_CHAIN (*t);
+	nsubblocks ++;
+      }
+   /* Outer scope is always used.  */
+   if (!BLOCK_SUPERCONTEXT (scope)
+       || TREE_CODE (BLOCK_SUPERCONTEXT (scope)) == FUNCTION_DECL)
+     unused = false;
+   /* If there are more than one live subblocks, it is used.  */
+   else if (nsubblocks > 1)
+     unused = false;
+   /* When there is only one subblock, see if it is just wrapper we can
+      ignore.  Wrappers are not declaring any variables and not changing
+      abstract origin.  */
+   else if (nsubblocks == 1
+	    && (BLOCK_VARS (scope)
+		|| ((debug_info_level == DINFO_LEVEL_NORMAL
+		     || debug_info_level == DINFO_LEVEL_VERBOSE)
+		    && ((BLOCK_ABSTRACT_ORIGIN (scope)
+			!= BLOCK_ABSTRACT_ORIGIN (BLOCK_SUPERCONTEXT (scope)))))))
+     unused = false;
+   return unused;
+}
 
 /* Mark all VAR_DECLS under *EXPR_P as used, so that they won't be 
    eliminated during the tree->rtl conversion process.  */
@@ -452,6 +565,7 @@ remove_unused_locals (void)
   referenced_var_iterator rvi;
   var_ann_t ann;
 
+  mark_scope_block_unused (DECL_INITIAL (current_function_decl));
   /* Assume all locals are unused.  */
   FOR_EACH_REFERENCED_VAR (t, rvi)
     var_ann (t)->used = false;
@@ -498,7 +612,6 @@ remove_unused_locals (void)
 	  *cell = TREE_CHAIN (*cell);
 	  continue;
 	}
-
       cell = &TREE_CHAIN (*cell);
     }
 
@@ -516,6 +629,7 @@ remove_unused_locals (void)
 	&& !ann->symbol_mem_tag
 	&& !TREE_ADDRESSABLE (t))
       remove_referenced_var (t);
+  remove_unused_scope_block_p (DECL_INITIAL (current_function_decl));
 }
 
 

Re: Put scope blocks on a diet

[Diego Novillo]

On 7/24/07 2:02 PM, Jan Hubicka wrote:

> 	* gimple-low.c (lower_stmt): When not doing debugging, don't save
> 	info about block.
> 	* tree-ssa-live.c: Include debug.h and flags.h.
> 	(mark_scope_block_unused): New function.
> 	(remove_unused_scope_block_p): New function.
> 	(remove_unused_locals): Remove unused blocks too.

OK with a few minor changes

> Index: gimple-low.c
> ===================================================================
> --- gimple-low.c	(revision 126859)
> +++ gimple-low.c	(working copy)
> @@ -210,7 +210,9 @@ lower_stmt (tree_stmt_iterator *tsi, str
>  {
>    tree stmt = tsi_stmt (*tsi);
>  
> -  if (EXPR_HAS_LOCATION (stmt) && data)
> +  if (EXPR_HAS_LOCATION (stmt) && data

&& data on the next line

>    tree t = *tp;
> +  char const c = TREE_CODE_CLASS (TREE_CODE (t));

Hmm?  TREE_CODE_CLASS is of type enum tree_code_class.

>  
> +/* Mark the scope block SCOPE and is subblocks unused when they can be

s/is/its/

> +
> +/* Look if the block is dead (by possibly elliminating it's dead subblocks)

s/elliminating/eliminating/
s/it's/its/

> +   and return true if so.  
> +   Block is declared dead if:
> +     1) No statements are associated with it.
> +     2) Declares no live variables
> +     3) All subblocks are dead
> +	or there is precisely one subblocks and the block
> +	has same abstract origin as outer block and declares
> +	no variables, so it is pure wrapper.
> +   When we are not outputting full debug info, we also elliminate dead variables
> +   out of scope blocks to let them to be recycled by GGC and to save copying work
> +   done by the inliner.
> +*/

Closing comment on previous line.

> +  mark_scope_block_unused (DECL_INITIAL (current_function_decl));

Comment before '/* Assume every block in the function is unused.  */'

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Jul 24, 2007, Jan Hubicka <jh@suse.cz> wrote:

> With debugging disabled gimplifier also knows to not assign statements
> to block so we can remove more of them.  This seems to be safe to do
> concerning consistency in between -g and no -g (we still keep enough
> block around to support some of other tests made)

Is it a hunch that it is safe, or do you have any actual arguments to
justify this claim?  It's actually caused major regressions to
bootstrap-debug (introduced with the yet-to-be-submitted patch below)
on a tree in which I have a large patch in progress that adds debug
annotations without any changes to the generated code.

I still haven't tried a bootstrap-debug with your patch but without
mine, and I don't know whether we'd pass bootstrap-debug before our
patches, but over the past two weeks I've come to appreciate how easy
it is to get changes into the compiler output by adding no-effect
statements all over the place ;-)

When inlining refrains from copying decls, this affects DECL_UIDs,
which affects the placement of decls in hash tables, which affects the
order in which they're visited in hash table walks, which affects SSA
version numbering, and in the end we optimize the code differently
(coalescing comes to mind), breaking the -g invariant.

Would you verify that your references to the debug level don't break
bootstrap-debug?  Thanks in advance,


I've left the Makefile.in out of the patch below, because it's such a
large (200k) repetitive change.  This means you have to run autogen
after installing the patch to get a bootstrap-debug target.


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

	* Makefile.def (STAGE2_CFLAGS, STAGE3_CFLAGS, STAGE4_CFLAGS):
	Add to flags_to_pass.  Adjust uses of BOOT_CFLAGS.
	(bootstrap2-debug, bootstrap-debug): New bootstrap stages.
	* Makefile.tpl (STAGE2_CFLAGS, STAGE3_CFLAGS, STAGE4_CFLAGS): New.
	(do-compare, do-compare3, do-compare-debug): New.
	([+compare-target+]): Use them.

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

	* compare-debug: New.

Index: Makefile.def
===================================================================
--- Makefile.def.orig	2007-07-31 18:44:50.000000000 -0300
+++ Makefile.def	2007-07-31 18:46:57.000000000 -0300
@@ -4,7 +4,7 @@ AutoGen definitions Makefile.tpl;
 // Makefile.in is generated from Makefile.tpl by 'autogen Makefile.def'.
 // This file was originally written by Nathanael Nerode.
 //
-//   Copyright 2002, 2003, 2004, 2005, 2006 Free Software Foundation
+//   Copyright 2002, 2003, 2004, 2005, 2006, 2007 Free Software Foundation
 //
 // This file is free software; you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -236,6 +236,9 @@ flags_to_pass = { flag= LIBCXXFLAGS ; };
 flags_to_pass = { flag= STAGE1_CFLAGS ; };
 flags_to_pass = { flag= STAGE1_CHECKING ; };
 flags_to_pass = { flag= STAGE1_LANGUAGES ; };
+flags_to_pass = { flag= STAGE2_CFLAGS ; };
+flags_to_pass = { flag= STAGE3_CFLAGS ; };
+flags_to_pass = { flag= STAGE4_CFLAGS ; };
 flags_to_pass = { flag= GNATBIND ; };
 flags_to_pass = { flag= GNATMAKE ; };
 
@@ -512,26 +515,38 @@ bootstrap_stage = {
 	id=2 ; prev=1 ;
 	bootstrap_target=bootstrap2 ;
 	stage_configure_flags="@stage2_werror_flag@" ;
-	stage_cflags="$(BOOT_CFLAGS)" ; };
+	stage_cflags="$(STAGE2_CFLAGS)" ; };
+bootstrap_stage = {
+	id=b2g0 ; prev=1 ;
+	bootstrap_target=bootstrap2-debug ;
+	stage_configure_flags="@stage2_werror_flag@" ;
+	stage_cflags="$(STAGE2_CFLAGS) -g0" ; };
 bootstrap_stage = {
 	id=3 ; prev=2 ; lean=1 ;
 	compare_target=compare ;
 	bootstrap_target=bootstrap ;
 	cleanstrap_target=cleanstrap ;
 	stage_configure_flags="@stage2_werror_flag@" ;
-	stage_cflags="$(BOOT_CFLAGS)" ; };
+	stage_cflags="$(STAGE3_CFLAGS)" ; };
+bootstrap_stage = {
+	id=b3g2 ; prev=b2g0 ; lean=1 ;
+	compare_target=compare-debug ;
+	bootstrap_target=bootstrap-debug ;
+	cleanstrap_target=cleanstrap-debug ;
+	stage_configure_flags="@stage2_werror_flag@" ;
+	stage_cflags="$(STAGE3_CFLAGS) -g2" ; };
 bootstrap_stage = {
 	id=4 ; prev=3 ; lean=2 ;
 	compare_target=compare3 ;
 	bootstrap_target=bootstrap4 ;
 	stage_configure_flags="@stage2_werror_flag@" ;
-	stage_cflags="$(BOOT_CFLAGS)" ; };
+	stage_cflags="$(STAGE4_CFLAGS)" ; };
 bootstrap_stage = {
 	id=profile ; prev=1 ;
 	stage_configure_flags="@stage2_werror_flag@" ;
-	stage_cflags='$(BOOT_CFLAGS) -fprofile-generate' ; };
+	stage_cflags='$(STAGE2_CFLAGS) -fprofile-generate' ; };
 bootstrap_stage = {
 	id=feedback ; prev=profile ;
 	bootstrap_target=profiledbootstrap ;
 	stage_configure_flags="@stage2_werror_flag@" ;
-	stage_cflags='$(BOOT_CFLAGS) -fprofile-use' ; };
+	stage_cflags='$(STAGE3_CFLAGS) -fprofile-use' ; };
Index: Makefile.tpl
===================================================================
--- Makefile.tpl.orig	2007-07-31 18:44:50.000000000 -0300
+++ Makefile.tpl	2007-07-31 18:46:57.000000000 -0300
@@ -6,7 +6,7 @@ in
 #
 # Makefile for directory with subdirs to build.
 #   Copyright (C) 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-#   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006 Free Software Foundation
+#   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007 Free Software Foundation
 #
 # This file is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -323,6 +323,14 @@ STAGE1_CFLAGS=@stage1_cflags@
 STAGE1_CHECKING=@stage1_checking@
 STAGE1_LANGUAGES=@stage1_languages@
 
+STAGE2_CFLAGS=$(BOOT_CFLAGS)
+STAGE3_CFLAGS=$(BOOT_CFLAGS)
+STAGE4_CFLAGS=$(BOOT_CFLAGS)
+
+do-compare = @do_compare@
+do-compare3 = $(do-compare)
+do-compare-debug = $(SHELL) $(srcdir)/contrib/compare-debug $$f1 $$f2
+
 # -----------------------------------------------
 # Programs producing files for the TARGET machine
 # -----------------------------------------------
@@ -1316,7 +1324,7 @@ do-clean: clean-stage[+id+]
 	cd .. ; \
 	for file in $${files} ; do \
 	  f1=$$r/stage[+prev+]-gcc/$$file; f2=$$r/stage[+id+]-gcc/$$file; \
-	  @do_compare@ > /dev/null 2>&1; \
+	  $(do-[+compare-target+]) > /dev/null 2>&1; \
 	  if test $$? -eq 1; then \
 	    case $$file in \
 	      ./cc*-checksum$(objext) | ./libgcc/* ) \
Index: contrib/compare-debug
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ contrib/compare-debug	2007-08-02 05:44:22.000000000 -0300
@@ -0,0 +1,67 @@
+#! /bin/sh
+
+# Compare stripped copies of two given object files.
+
+# Copyright (C) 2007 Free Software Foundation
+# Originally by Alexandre Oliva <aoliva@redhat.com>
+
+# This file is part of GCC.
+
+# GCC is free software; you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation; either version 3, or (at your option) any later
+# version.
+
+# GCC is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+# or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+# License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with GCC; see the file COPYING3.  If not see
+# <http://www.gnu.org/licenses/>.
+
+if test $# != 2; then
+  echo 'usage: compare-debug file1.o file2.o' >&2
+  exit 1
+fi
+
+if test ! -f "$1"; then
+  echo "$1" does not exist >&2
+  exit 1
+fi
+
+if test ! -f "$2"; then
+  echo "$2" does not exist >&2
+  exit 1
+fi
+
+if test -f "$1".stripped; then
+  echo "$1".stripped already exists, overwriting >&2
+  exit 1
+fi
+
+if test -f "$2".stripped; then
+  echo "$2".stripped already exists, overwriting >&2
+  exit 1
+fi
+
+trap 'rm -f "$1".stripped "$2".stripped' 0 1 2 15
+
+cp "$1" "$1".stripped
+strip "$1".stripped
+
+cp "$2" "$2".stripped
+strip "$2".stripped
+
+if cmp "$1".stripped "$2".stripped; then
+  status=0
+else
+  status=1
+fi
+
+rm -f "$1".stripped "$2".stripped
+
+trap "exit $status; exit" 0 1 2 15
+
+exit $status


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Aug  2, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jul 24, 2007, Jan Hubicka <jh@suse.cz> wrote:
>> With debugging disabled gimplifier also knows to not assign statements
>> to block so we can remove more of them.  This seems to be safe to do
>> concerning consistency in between -g and no -g (we still keep enough
>> block around to support some of other tests made)

> Is it a hunch that it is safe, or do you have any actual arguments to
> justify this claim?  It's actually caused major regressions to
> bootstrap-debug (introduced with the yet-to-be-submitted patch below)
> on a tree in which I have a large patch in progress that adds debug
> annotations without any changes to the generated code.

I reverted my large patch, ran bootstrap-debug, and it failed
spectacularly.  Then I installed this patch, started over, and
bootstrap-debug passed (both with and without my patch).  On
x86_64-linux-gnu.

I realize this kills some of the memory savings you hoped for, but -g
must not ever modify the generated code.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-unused-scope-block.patch --]
[-- Type: text/x-patch, Size: 2133 bytes --]

Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c.orig	2007-08-02 15:50:35.000000000 -0300
+++ gcc/tree-ssa-live.c	2007-08-02 18:09:46.000000000 -0300
@@ -468,39 +468,10 @@ mark_scope_block_unused (tree scope)
 static bool
 remove_unused_scope_block_p (tree scope)
 {
-  tree *t, *next;
+  tree *t;
   bool unused = !TREE_USED (scope);
-  var_ann_t ann;
   int nsubblocks = 0;
 
-  for (t = &BLOCK_VARS (scope); *t; t = next)
-    {
-      next = &TREE_CHAIN (*t);
-
-      /* Debug info of nested function reffers to the block of the
-	 function.  */
-      if (TREE_CODE (*t) == FUNCTION_DECL)
-	unused = false;
-
-      /* When we are outputting debug info, we usually want to output
-	 info about optimized-out variables in the scope blocks.
-	 Exception are the scope blocks not containing any instructions
-	 at all so user can't get into the scopes at first place.  */
-      else if ((ann = var_ann (*t)) != NULL
-		&& ann->used)
-	unused = false;
-
-      /* When we are not doing full debug info, we however can keep around
-	 only the used variables for cfgexpand's memory packing saving quite
-	 a lot of memory.  */
-      else if (debug_info_level != DINFO_LEVEL_NORMAL
-	       && debug_info_level != DINFO_LEVEL_VERBOSE)
-	{
-	  *t = TREE_CHAIN (*t);
-	  next = t;
-	}
-    }
-
   for (t = &BLOCK_SUBBLOCKS (scope); *t ;)
     if (remove_unused_scope_block_p (*t))
       {
@@ -533,12 +504,10 @@ remove_unused_scope_block_p (tree scope)
    /* When there is only one subblock, see if it is just wrapper we can
       ignore.  Wrappers are not declaring any variables and not changing
       abstract origin.  */
-   else if (nsubblocks == 1
+   else if (nsubblocks <= 1
 	    && (BLOCK_VARS (scope)
-		|| ((debug_info_level == DINFO_LEVEL_NORMAL
-		     || debug_info_level == DINFO_LEVEL_VERBOSE)
-		    && ((BLOCK_ABSTRACT_ORIGIN (scope)
-			!= BLOCK_ABSTRACT_ORIGIN (BLOCK_SUPERCONTEXT (scope)))))))
+		|| (BLOCK_ABSTRACT_ORIGIN (scope)
+		    != BLOCK_ABSTRACT_ORIGIN (BLOCK_SUPERCONTEXT (scope)))))
      unused = false;
    return unused;
 }

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> I reverted my large patch, ran bootstrap-debug, and it failed
> spectacularly.  Then I installed this patch, started over, and
> bootstrap-debug passed (both with and without my patch).  On
> x86_64-linux-gnu.
> 
> I realize this kills some of the memory savings you hoped for, but -g
> must not ever modify the generated code.

Hi,
you seem to be removing a loop trying to prove block used that should
not make debug output more acurate at all.  At the same time you seem
to make check for abstract origins to match unconditional that might end
up in making compilation more accurate.
I will try to look into what caused the particular changes you see with
your bootstrap patch (that seems like great idea overall)
I did some limited testing with my patch same as you do - compiling with
and without debugging a comparing stripped binary but apparently I've
missed this problem.
Only algorithm in compiler that depends on block structure I know of is
the way stack frame usage is allocated in cfgexpand, perhaps we manage
to produce some unstability there.

Honza

Re: Put scope blocks on a diet

[Jan Hubicka]

> On Aug  2, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
> > On Jul 24, 2007, Jan Hubicka <jh@suse.cz> wrote:
> >> With debugging disabled gimplifier also knows to not assign statements
> >> to block so we can remove more of them.  This seems to be safe to do
> >> concerning consistency in between -g and no -g (we still keep enough
> >> block around to support some of other tests made)
> 
> > Is it a hunch that it is safe, or do you have any actual arguments to
> > justify this claim?  It's actually caused major regressions to
> > bootstrap-debug (introduced with the yet-to-be-submitted patch below)
> > on a tree in which I have a large patch in progress that adds debug
> > annotations without any changes to the generated code.
> 
> I reverted my large patch, ran bootstrap-debug, and it failed
> spectacularly.  Then I installed this patch, started over, and
> bootstrap-debug passed (both with and without my patch).  On
> x86_64-linux-gnu.
> 
> I realize this kills some of the memory savings you hoped for, but -g
> must not ever modify the generated code.
Hi,
I would like to test the attached patch.  It prevents the removal before
inlining so UIDs should remain the same.  However I do get massive
regressions on bootstrap-debug with or without the block removal code.
I will now rebuild and see if the set of failing files changes by 
disabling block removal, but if you do have some testcases handy, it
would help.

I wonder if we need to keep those at all - dwarf2 already uses abstract
origin to avoid need to saveinfo on variables in each copy.  We can
probably just keep around the dead variables on separate list in their
ABSTRACT_ORIGIN -g or not?

Honza

Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c	(revision 127183)
+++ tree-ssa-live.c	(working copy)
@@ -494,7 +495,11 @@ remove_unused_scope_block_p (tree scope)
 	 only the used variables for cfgexpand's memory packing saving quite
 	 a lot of memory.  */
       else if (debug_info_level != DINFO_LEVEL_NORMAL
-	       && debug_info_level != DINFO_LEVEL_VERBOSE)
+	       && debug_info_level != DINFO_LEVEL_VERBOSE
+	       /* Removing declarations before inlining is going to affect
+		  DECL_UID that in turn is going to affect hashtables and
+		  code generation.  */
+	       && cfun->after_inlining)
 	{
 	  *t = TREE_CHAIN (*t);
 	  next = t;

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Aug  6, 2007, Jan Hubicka <jh@suse.cz> wrote:

> I would like to test the attached patch.  It prevents the removal before
> inlining so UIDs should remain the same.  However I do get massive
> regressions on bootstrap-debug with or without the block removal code.

http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01047.html might help.

> I wonder if we need to keep those at all - dwarf2 already uses abstract
> origin to avoid need to saveinfo on variables in each copy.  We can
> probably just keep around the dead variables on separate list in their
> ABSTRACT_ORIGIN -g or not?

Anything that enables us to maintain the decl uids in sync should
work.  So, if, instead of deleting decls or even complete blocks in
the blocks tree, we replace them by something that directs the inliner
to increment next_decl_uids by a certain amount at that point, this
should not affect code generation.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Aug  6, 2007, Jan Hubicka <jh@suse.cz> wrote:

> I would like to test the attached patch.  It prevents the removal before
> inlining so UIDs should remain the same.  However I do get massive
> regressions on bootstrap-debug with or without the block removal code.
> I will now rebuild and see if the set of failing files changes by 
> disabling block removal, but if you do have some testcases handy, it
> would help.

> I wonder if we need to keep those at all - dwarf2 already uses abstract
> origin to avoid need to saveinfo on variables in each copy.  We can
> probably just keep around the dead variables on separate list in their
> ABSTRACT_ORIGIN -g or not?

This patch of yours (except for the ChangeLog entry) fixes the
-g-changes-generated-code in the trunk, so this should go in.

However, it breaks the vta branch, because we still keep debug
annotations for variables in blocks that your patch removes.  I
suppose I could just drop such annotations in the floor for now, but,
in the long run, should I?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-unused-scope-block-alt.patch --]
[-- Type: text/x-patch, Size: 884 bytes --]

for gcc/ChangeLog.vta
from  Jan Hubicka  <jh@suse.cz>

	* tree-ssa-live.c (remove_unused_scope_block_p): Drop
	declarations and blocks only after inlining.

Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c	(revision 127183)
+++ gcc/tree-ssa-live.c	(working copy)
@@ -494,7 +495,11 @@ remove_unused_scope_block_p (tree scope)
 	 only the used variables for cfgexpand's memory packing saving quite
 	 a lot of memory.  */
       else if (debug_info_level != DINFO_LEVEL_NORMAL
-	       && debug_info_level != DINFO_LEVEL_VERBOSE)
+	       && debug_info_level != DINFO_LEVEL_VERBOSE
+	       /* Removing declarations before inlining is going to affect
+		  DECL_UID that in turn is going to affect hashtables and
+		  code generation.  */
+	       && cfun->after_inlining)
 	{
 	  *t = TREE_CHAIN (*t);
 	  next = t;

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> On Aug  6, 2007, Jan Hubicka <jh@suse.cz> wrote:
> 
> > I would like to test the attached patch.  It prevents the removal before
> > inlining so UIDs should remain the same.  However I do get massive
> > regressions on bootstrap-debug with or without the block removal code.
> > I will now rebuild and see if the set of failing files changes by 
> > disabling block removal, but if you do have some testcases handy, it
> > would help.
> 
> > I wonder if we need to keep those at all - dwarf2 already uses abstract
> > origin to avoid need to saveinfo on variables in each copy.  We can
> > probably just keep around the dead variables on separate list in their
> > ABSTRACT_ORIGIN -g or not?
> 
> This patch of yours (except for the ChangeLog entry) fixes the
> -g-changes-generated-code in the trunk, so this should go in.

Thanks for confirmation.  I was still planning to re-test it but didn't
get around it yet.
> 
> However, it breaks the vta branch, because we still keep debug
> annotations for variables in blocks that your patch removes.  I
> suppose I could just drop such annotations in the floor for now, but,
> in the long run, should I?


The pass is basically walking the function body looking for what scope
blocks are used (ie having some statements in or some variables declared
live).  If you have another annotations (I didn't looked into your code
yet), I guess you just need to update the marking stage to prevent them
from being removed if they still might end up being output to debug
info.

Honza
> 

> for gcc/ChangeLog.vta
> from  Jan Hubicka  <jh@suse.cz>
> 
> 	* tree-ssa-live.c (remove_unused_scope_block_p): Drop
> 	declarations and blocks only after inlining.
> 
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c	(revision 127183)
> +++ gcc/tree-ssa-live.c	(working copy)
> @@ -494,7 +495,11 @@ remove_unused_scope_block_p (tree scope)
>  	 only the used variables for cfgexpand's memory packing saving quite
>  	 a lot of memory.  */
>        else if (debug_info_level != DINFO_LEVEL_NORMAL
> -	       && debug_info_level != DINFO_LEVEL_VERBOSE)
> +	       && debug_info_level != DINFO_LEVEL_VERBOSE
> +	       /* Removing declarations before inlining is going to affect
> +		  DECL_UID that in turn is going to affect hashtables and
> +		  code generation.  */
> +	       && cfun->after_inlining)
>  	{
>  	  *t = TREE_CHAIN (*t);
>  	  next = t;

> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On 10/3/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> > I would like to test the attached patch.  It prevents the removal before
> > inlining so UIDs should remain the same.  However I do get massive
> > regressions on bootstrap-debug with or without the block removal code.
> > I will now rebuild and see if the set of failing files changes by
> > disabling block removal, but if you do have some testcases handy, it
> > would help.
>
> > I wonder if we need to keep those at all - dwarf2 already uses abstract
> > origin to avoid need to saveinfo on variables in each copy.  We can
> > probably just keep around the dead variables on separate list in their
> > ABSTRACT_ORIGIN -g or not?
>
> This patch of yours (except for the ChangeLog entry) fixes the
> -g-changes-generated-code in the trunk, so this should go in.
>
> However, it breaks the vta branch, because we still keep debug
> annotations for variables in blocks that your patch removes.  I
> suppose I could just drop such annotations in the floor for now, but,
> in the long run, should I?

In the long run, we should generate (and output) debug information
for types and declarations early and simply remember the DIE when
refering to it instead of keeping our hands on the types and decls
itself.

Richard.

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Oct  3, 2007, Jan Hubicka <jh@suse.cz> wrote:

> The pass is basically walking the function body looking for what scope
> blocks are used (ie having some statements in or some variables declared
> live).  If you have another annotations (I didn't looked into your code
> yet), I guess you just need to update the marking stage to prevent them
> from being removed if they still might end up being output to debug
> info.

The thing is that I have to make sure that preventing them from being
removed can't possibly modify the generated code.  It's not entirely
obvious to me that this is so, but I'll look into it.

Meanwhile, is it ok to install the patch?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> 
> In the long run, we should generate (and output) debug information
> for types and declarations early and simply remember the DIE when
> refering to it instead of keeping our hands on the types and decls
> itself.

This seems to be bit dificult to implement given that inlining and other
optimizations modify/build new types....
Would be OK to commit the discussed patch to fix the debug-bootstrap?

Honza
> 
> Richard.

Re: Put scope blocks on a diet

[Michael Matz]

Hi,

On Mon, 8 Oct 2007, Jan Hubicka wrote:

> > 
> > In the long run, we should generate (and output) debug information
> > for types and declarations early and simply remember the DIE when
> > refering to it instead of keeping our hands on the types and decls
> > itself.
> 
> This seems to be bit dificult to implement given that inlining and other
> optimizations modify/build new types....

I certainly hope that middle-end transformation don't build many new 
types.  Actually I sure hope they build no new types at all, but I'll 
better not say none, you never know :)


Ciao,
Michael.

Re: Put scope blocks on a diet

[Jan Hubicka]

> Hi,
> 
> On Mon, 8 Oct 2007, Jan Hubicka wrote:
> 
> > > 
> > > In the long run, we should generate (and output) debug information
> > > for types and declarations early and simply remember the DIE when
> > > refering to it instead of keeping our hands on the types and decls
> > > itself.
> > 
> > This seems to be bit dificult to implement given that inlining and other
> > optimizations modify/build new types....
> 
> I certainly hope that middle-end transformation don't build many new 
> types.  Actually I sure hope they build no new types at all, but I'll 
> better not say none, you never know :)

What I had in mind is that inlining is building copies of types in new
copied scopes, that also needs DIEs and since we are slowly getting into
busyness of transforming datastructures (like in struct reorg), we
should be able to compensate this in debug info instead of emitting
debug info for original layout and writting data in modified.

Honza
> 
> 
> Ciao,
> Michael.

Re: Put scope blocks on a diet

[Michael Matz]

Hi,

On Mon, 8 Oct 2007, Jan Hubicka wrote:

> > I certainly hope that middle-end transformation don't build many new 
> > types.  Actually I sure hope they build no new types at all, but I'll 
> > better not say none, you never know :)
> 
> What I had in mind is that inlining is building copies of types in new 
> copied scopes, that also needs DIEs

Yes, for these transformations we need to transform the DIEs with them (in 
this case make them also be referred from new locations, although this 
should fall out naturally in this case, as the new scope blocks will 
contain new decls referring to existing types, hence they can be 
associated with those new scopes too).

> and since we are slowly getting into busyness of transforming 
> datastructures (like in struct reorg), we should be able to compensate 
> this in debug info instead of emitting debug info for original layout 
> and writting data in modified.

Indeed.  That requires creating new DIEs then.  To me it's unclear if that 
will turn out bad.


Ciao,
Michael.

Re: Put scope blocks on a diet

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Oct  3, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> However, it breaks the vta branch, because we still keep debug
> annotations for variables in blocks that your patch removes.  I
> suppose I could just drop such annotations in the floor for now, but,
> in the long run, should I?

Rather than dropping the annotations, I can simply drop the block
information from them, as in the following patch, that I've just
installed in the vta branch, along with your patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-unused-scope-block-fixup.patch --]
[-- Type: text/x-patch, Size: 784 bytes --]

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

	* tree-ssa-live.c (copy_body_r): Cope with references to
	removed blocks in debug stmts.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-09 03:31:45.000000000 -0300
+++ gcc/tree-inline.c	2007-10-09 17:39:25.000000000 -0300
@@ -731,8 +731,12 @@ copy_body_r (tree *tp, int *walk_subtree
 	      tree *n;
 	      n = (tree *) pointer_map_contains (id->decl_map,
 						 TREE_BLOCK (*tp));
-	      gcc_assert (n);
-	      new_block = *n;
+	      if (n)
+		new_block = *n;
+	      else if (IS_DEBUG_STMT (*tp) || processing_debug_stmt_p)
+		new_block = NULL;
+	      else
+		gcc_unreachable ();
 	    }
 	  TREE_BLOCK (*tp) = new_block;
 	}

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Oct  9, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Oct  3, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

>> However, it breaks the vta branch, because we still keep debug
>> annotations for variables in blocks that your patch removes.  I
>> suppose I could just drop such annotations in the floor for now, but,
>> in the long run, should I?

> Rather than dropping the annotations, I can simply drop the block
> information from them, as in the following patch, that I've just
> installed in the vta branch, along with your patch.

This was enough to get bootstrap to pass, but not bootstrap-debug.  As
expected (but forgotten by myself), bootstrap-debug would hit
differences in java/jcf-parse.o and tree-ssa-loop-ivcanon.o because of
variables that were removed from regular code and logical blocks, but
that remained in debug stmts.  This patch fixes it.  Installed in the
vta branch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-unused-blocks-drop-stmts.patch --]
[-- Type: text/x-patch, Size: 1131 bytes --]

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

	* tree-inline.c (copy_debug_stmt): Drop stmt that refers to
	unmapped variable.

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-09 17:47:01.000000000 -0300
+++ gcc/tree-inline.c	2007-10-10 04:50:33.000000000 -0300
@@ -1368,13 +1368,21 @@ copy_cfg_body (copy_body_data * id, gcov
 static void
 copy_debug_stmt (tree stmt, copy_body_data *id)
 {
-  tree t;
+  tree *tp;
 
   processing_debug_stmt_p = true;
 
-  t = VAR_DEBUG_VALUE_VAR (stmt);
-  walk_tree (&t, copy_body_r, id, NULL);
-  VAR_DEBUG_VALUE_SET_VAR (stmt, t);
+  tp = (tree *)pointer_map_contains (id->decl_map, VAR_DEBUG_VALUE_VAR (stmt));
+  if (!tp)
+    {
+      /* This debug stmt refers to a variable that was completely
+	 optimized away.  Drop it.  */
+      block_stmt_iterator bsi = bsi_for_stmt (stmt);
+      bsi_remove (&bsi, true);
+      processing_debug_stmt_p = false;
+      return;
+    }
+  VAR_DEBUG_VALUE_SET_VAR (stmt, *tp);
 
   walk_tree (&VAR_DEBUG_VALUE_VALUE (stmt), copy_body_r, id, NULL);
 

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Oct  3, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Aug  6, 2007, Jan Hubicka <jh@suse.cz> wrote:

>> It prevents the removal before
>> inlining so UIDs should remain the same.

> This patch of yours (except for the ChangeLog entry) fixes the
> -g-changes-generated-code in the trunk, so this should go in.

> However, it breaks the vta branch, because we still keep debug
> annotations for variables in blocks that your patch removes.  I
> suppose I could just drop such annotations in the floor for now, but,
> in the long run, should I?

I did this, but then I wonder...  How come the differences in decl
uids arise from inlining of removed variables, if the patch was
supposed to take effect only after inlining?

As it turns out, looking at the code, I get the impression that
cfun->after_inline determines whether the function was inlined into,
rather than whether it has already been inlined into every point it
would be throughout the current compilation.  Is this not so?

If it is, then the test is meaningless.  The problem arises when we
change the declarations in a function's logical blocks and then inline
into others.

What am I missing?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> 
> I did this, but then I wonder...  How come the differences in decl
> uids arise from inlining of removed variables, if the patch was
> supposed to take effect only after inlining?
> 
> As it turns out, looking at the code, I get the impression that
> cfun->after_inline determines whether the function was inlined into,

cfun->after_inline is set after fixup_cfg that removes extra edges from
cfg otherwise needed by inliner, so we really ought not to inline that
function after that point.
On mainline fixup_cfg is executed after apply_inline, so it should be
safe.

Honza
> rather than whether it has already been inlined into every point it
> would be throughout the current compilation.  Is this not so?
> 
> If it is, then the test is meaningless.  The problem arises when we
> change the declarations in a function's logical blocks and then inline
> into others.
> 
> What am I missing?
> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On Oct 10, 2007, Jan Hubicka <hubicka@ucw.cz> wrote:

>> I did this, but then I wonder...  How come the differences in decl
>> uids arise from inlining of removed variables, if the patch was
>> supposed to take effect only after inlining?

> cfun->after_inline is set after fixup_cfg [...]
> On mainline fixup_cfg is executed after apply_inline

Indeed.

>> If it is, then the test is meaningless.  The problem arises when we
>> change the declarations in a function's logical blocks and then inline
>> into others.

>> What am I missing?

That remove_unused_locals() was called by other post-pass TODOs, and
that it would remove blocks that didn't contain any sub-blocks but
that did contain declarations.  This caused variables only referenced
in debug stmts to be wiped away too early.

This alternate patch arranges for such blocks to not be deleted before
inlining.  I'm not entirely sure removing them too early might
possibly result in changes to the generated code, but I can't think of
a case in which it would, with the current (trunk) debug info
infrastructure.

Thoughts?

Here's the patch I'm testing now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-debug-unused-scope-block-alt.patch --]
[-- Type: text/x-patch, Size: 1969 bytes --]

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>, Jan Hubicka  <jh@suse.cz>

	* tree-ssa-live.c (remove_unused_scope_block_p): Drop
	declarations and blocks only after inlining.  Check that
	non-empty blocks are not dropped.
	* tree-inline.c (expand_call_inline): Check that functions are
	not inlined too late.

Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c.orig	2007-10-07 19:57:10.000000000 -0300
+++ gcc/tree-ssa-live.c	2007-10-11 02:47:17.000000000 -0300
@@ -493,8 +493,15 @@ remove_unused_scope_block_p (tree scope)
       /* When we are not doing full debug info, we however can keep around
 	 only the used variables for cfgexpand's memory packing saving quite
 	 a lot of memory.  */
-      else if (debug_info_level != DINFO_LEVEL_NORMAL
-	       && debug_info_level != DINFO_LEVEL_VERBOSE)
+      else if (debug_info_level == DINFO_LEVEL_NORMAL
+	       || debug_info_level == DINFO_LEVEL_VERBOSE
+	       /* Removing declarations before inlining is going to affect
+		  DECL_UID that in turn is going to affect hashtables and
+		  code generation.  */
+	       || !cfun->after_inlining)
+	unused = false;
+
+      else
 	{
 	  *t = TREE_CHAIN (*t);
 	  next = t;
@@ -516,7 +523,10 @@ remove_unused_scope_block_p (tree scope)
 	    nsubblocks ++;
 	  }
 	else
-          *t = BLOCK_CHAIN (*t);
+	  {
+	    gcc_assert (!BLOCK_VARS (*t));
+	    *t = BLOCK_CHAIN (*t);
+	  }
       }
     else
       {
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2007-10-11 02:44:04.000000000 -0300
+++ gcc/tree-inline.c	2007-10-11 02:49:00.000000000 -0300
@@ -2640,6 +2640,8 @@ expand_call_inline (basic_block bb, tree
   id->src_cfun = DECL_STRUCT_FUNCTION (fn);
   id->call_expr = t;
 
+  gcc_assert (!id->src_cfun->after_inlining);
+
   initialize_inlined_parameters (id, t, fn, bb);
 
   if (DECL_INITIAL (fn))

[-- Attachment #3: Type: text/plain, Size: 250 bytes --]



-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On 10/11/07, Alexandre Oliva <aoliva@redhat.com> wrote:
> >> I did this, but then I wonder...  How come the differences in decl
> >> uids arise from inlining of removed variables, if the patch was
> >> supposed to take effect only after inlining?
>
> > cfun->after_inline is set after fixup_cfg [...]
> > On mainline fixup_cfg is executed after apply_inline
>
> Indeed.
>
> >> If it is, then the test is meaningless.  The problem arises when we
> >> change the declarations in a function's logical blocks and then inline
> >> into others.
>
> >> What am I missing?
>
> That remove_unused_locals() was called by other post-pass TODOs, and
> that it would remove blocks that didn't contain any sub-blocks but
> that did contain declarations.  This caused variables only referenced
> in debug stmts to be wiped away too early.
>
> This alternate patch arranges for such blocks to not be deleted before
> inlining.  I'm not entirely sure removing them too early might
> possibly result in changes to the generated code, but I can't think of
> a case in which it would, with the current (trunk) debug info
> infrastructure.
>
> Thoughts?
>
> Here's the patch I'm testing now.

This doesn't change the amount of scope blocks we remove, just the point in
time, right?  (That is, you mainly assert that we don't remove scopes with
declarations)

I'm worried that delaying deletion of scope blocks until after final
inlining will
increase peak memory usage as quite possibly the most savings currently
occur during early optimization.

Btw, did you analyze why

+              /* Removing declarations before inlining is going to affect
+                 DECL_UID that in turn is going to affect hashtables and
+                 code generation.  */
+              || !cfun->after_inlining)

removing declarations causes DECL_UID differences?  We don't re-use UIDs,
so this should be a no-op.  The only thing is that some algorithms walk the
referenced_vars hash-table and the order of this walk depends on the hash-table
size - but this should be really fixed in a different way (by removing
hash-table
walks).  But maybe the DECL_UID problem is a different one?

Thanks,
Richard.

Re: Put scope blocks on a diet

[Jan Hubicka]

> > This alternate patch arranges for such blocks to not be deleted before
> > inlining.  I'm not entirely sure removing them too early might
> > possibly result in changes to the generated code, but I can't think of
> > a case in which it would, with the current (trunk) debug info
> > infrastructure.
> >
> > Thoughts?
> >
> > Here's the patch I'm testing now.
> 
> This doesn't change the amount of scope blocks we remove, just the point in
> time, right?  (That is, you mainly assert that we don't remove scopes with
> declarations)
> 
> I'm worried that delaying deletion of scope blocks until after final
> inlining will
> increase peak memory usage as quite possibly the most savings currently
> occur during early optimization.

Delaying deleting will deifnitly increase the peak memory - but we will
have same peak when compiling with -g, so I would not worry about it
that much.  My primary motivation was to get memory usage down for both
debugging or nondebugging compilation.  Getting some extra score for
compilation without -g was just a bonus that seemed easy enough.
> 
> Btw, did you analyze why
> 
> +              /* Removing declarations before inlining is going to affect
> +                 DECL_UID that in turn is going to affect hashtables and
> +                 code generation.  */
> +              || !cfun->after_inlining)

Actually I think your proposed patch (modulo the asserts) is identical
to one I sent in August, or did I missed something trivial?
I tought that one is already on your branch and just waiting for
approval to mainline.

> 
> removing declarations causes DECL_UID differences?  We don't re-use UIDs,
> so this should be a no-op.  The only thing is that some algorithms walk the
> referenced_vars hash-table and the order of this walk depends on the hash-table
> size - but this should be really fixed in a different way (by removing
> hash-table
> walks).  But maybe the DECL_UID problem is a different one?

The problem is that removing them before inlining prevents inlining from
copying them and we end up with DECL_UID divergence.

So overall patch (including the asserts) makes sense to me, but I can't
approve it.

Better solution would be to handle those dead declarations same way as
dwarf.  It already does compress the info by just pointing to abstract
origins instead of outputting full declarations. We can do the same.
Instead of building full blown DECL_UID clone, just keep record of
abstract origins we need to add or let dwarf2out to check the origin of
block and work it out itself.

Honza
> 
> Thanks,
> Richard.

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Oct 11, 2007, Jan Hubicka <jh@suse.cz> wrote:

>> +              /* Removing declarations before inlining is going to affect
>> +                 DECL_UID that in turn is going to affect hashtables and
>> +                 code generation.  */
>> +              || !cfun->after_inlining)

> Actually I think your proposed patch (modulo the asserts) is identical
> to one I sent in August, or did I missed something trivial?

The difference is that my patch sets "unused = false;" when we refrain
from removing an unused variable for the sake of debug info.

Without it, we may remove a leaf block that still contains
declarations, and this causes differences in declaration numbers for
inlining, although I don't have a testcase that shows the problem
except in the vta branch.

> I tought that one is already on your branch and just waiting for
> approval to mainline.

Yes, but I'm now convinced it is wrong, for it will unintentionally
remove declarations of unused variables before inlining by removing
their containing blocks, and this may result in different code *and*
harm debug info.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> On Oct 11, 2007, Jan Hubicka <jh@suse.cz> wrote:
> 
> >> +              /* Removing declarations before inlining is going to affect
> >> +                 DECL_UID that in turn is going to affect hashtables and
> >> +                 code generation.  */
> >> +              || !cfun->after_inlining)
> 
> > Actually I think your proposed patch (modulo the asserts) is identical
> > to one I sent in August, or did I missed something trivial?
> 
> The difference is that my patch sets "unused = false;" when we refrain
> from removing an unused variable for the sake of debug info.
> 
> Without it, we may remove a leaf block that still contains
> declarations, and this causes differences in declaration numbers for
> inlining, although I don't have a testcase that shows the problem
> except in the vta branch.

Uh, thanks, I didin't noticed that.
Honza
> 
> > I tought that one is already on your branch and just waiting for
> > approval to mainline.
> 
> Yes, but I'm now convinced it is wrong, for it will unintentionally
> remove declarations of unused variables before inlining by removing
> their containing blocks, and this may result in different code *and*
> harm debug info.
> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On Oct 12, 2007 7:13 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > On Oct 11, 2007, Jan Hubicka <jh@suse.cz> wrote:
> >
> > >> +              /* Removing declarations before inlining is going to affect
> > >> +                 DECL_UID that in turn is going to affect hashtables and
> > >> +                 code generation.  */
> > >> +              || !cfun->after_inlining)
> >
> > > Actually I think your proposed patch (modulo the asserts) is identical
> > > to one I sent in August, or did I missed something trivial?
> >
> > The difference is that my patch sets "unused = false;" when we refrain
> > from removing an unused variable for the sake of debug info.
> >
> > Without it, we may remove a leaf block that still contains
> > declarations, and this causes differences in declaration numbers for
> > inlining, although I don't have a testcase that shows the problem
> > except in the vta branch.
>
> Uh, thanks, I didin't noticed that.
> Honza
>
> >
> > > I tought that one is already on your branch and just waiting for
> > > approval to mainline.
> >
> > Yes, but I'm now convinced it is wrong, for it will unintentionally
> > remove declarations of unused variables before inlining by removing
> > their containing blocks, and this may result in different code *and*
> > harm debug info.

With

2007-11-26  Alexandre Oliva  <aoliva@redhat.com>, Jan Hubicka  <jh@suse.cz>

        * tree-ssa-live.c (remove_unused_scope_block_p): Drop
        declarations and blocks only after inlining.  Check that
        non-empty blocks are not dropped.
        * tree-inline.c (expand_call_inline): Check that functions are
        not inlined too late.

there is a 50% increase in memory consumption and a 5% increase in
compile-time for tramp3d.

I don't think this is acceptable.  Please revert the patch given that we are
late in stage3.

Thanks,
Richard.

Re: Put scope blocks on a diet

[Mark Mitchell]

Richard Guenther wrote:

> 2007-11-26  Alexandre Oliva  <aoliva@redhat.com>, Jan Hubicka  <jh@suse.cz>
> 
>         * tree-ssa-live.c (remove_unused_scope_block_p): Drop
>         declarations and blocks only after inlining.  Check that
>         non-empty blocks are not dropped.
>         * tree-inline.c (expand_call_inline): Check that functions are
>         not inlined too late.
> 
> there is a 50% increase in memory consumption and a 5% increase in
> compile-time for tramp3d.
> 
> I don't think this is acceptable.  Please revert the patch given that we are
> late in stage3.

I agree.  Let's take this out, and think again about how to solve this
problem.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> 2007-11-26  Alexandre Oliva  <aoliva@redhat.com>, Jan Hubicka  <jh@suse.cz>

>         * tree-ssa-live.c (remove_unused_scope_block_p): Drop
>         declarations and blocks only after inlining.  Check that
>         non-empty blocks are not dropped.
>         * tree-inline.c (expand_call_inline): Check that functions are
>         not inlined too late.

> there is a 50% increase in memory consumption and a 5% increase in
> compile-time for tramp3d.

For the record, two points:

1. this patch prevents non-empty blocks with referenced variables from
being dropped.  This was a bug, and it may have given the impression
that the memory reductions achieved through the earlier, buggy patch
that IIRC went in on July 24 was correct.  But dropping too much
information was a mistake.  We could decide it's ok and desirable to
drop such information, but it has to be a conscious decision, not an
accidental fallout from a bug.

2. this patch delays the garbage-collection of some declarations after
inlining, to preserve decl UIDs in sync with and without debug
information.  I personally think that keeping decl UIDs is sync is a
very desirable property, and it's been quite useful for my recent
debugging efforts, but there are other ways to do this than keeping
declarations around just so that they can be cloned at the right time.
This particular fix should not make any difference if you're compiling
with -g, except where bug 1. incorrectly dropped entire blocks.

> I don't think this is acceptable.  Please revert the patch given that we are
> late in stage3.

I don't think letting -g or -g0 affect the executable code output by
the compiler is acceptable.  Reverting this patch would bring back the
regression in this regard, that was introduced on July 24, 2007.  So,
shall I revert both, or is the bug that caused us to unintentionally
drop a lot of information, thereby exaggerating the memory savings
that could be accomplished through the patch, going to trample a
fundamental design principle of GCC, not to mention the ability for
GCC to stand a chance of generating debug information for the
unintentionally-discarded information?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Mark Mitchell]

Alexandre Oliva wrote:

>> there is a 50% increase in memory consumption and a 5% increase in
>> compile-time for tramp3d.

> For the record, two points:

Alexandre, please take the rhetoric down a notch.  I know you have
strong feelings about this, but you're among friends. :-)

> 1. this patch prevents non-empty blocks with referenced variables from
> being dropped.  This was a bug, and it may have given the impression
> that the memory reductions achieved through the earlier, buggy patch
> that IIRC went in on July 24 was correct.  But dropping too much
> information was a mistake.  We could decide it's ok and desirable to
> drop such information, but it has to be a conscious decision, not an
> accidental fallout from a bug.

That certainly makes sense.  Do you have a URL for the July 24 patch?

Meanwhile, please revert your patch.  We don't want to oscillate too
much.  Is there a PR for the problem that your patch fixed?  If not,
please sure that there is one.

> I don't think letting -g or -g0 affect the executable code output by
> the compiler is acceptable.

I don't either.  But, we don't have agreement on how best to solve the
problem.  So, let's a get PR open, which will help ensure we solve the
problem before the release, and then let's talk about how we can solve it.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 27, 2007 8:17 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>
> > 2007-11-26  Alexandre Oliva  <aoliva@redhat.com>, Jan Hubicka  <jh@suse.cz>
>
> >         * tree-ssa-live.c (remove_unused_scope_block_p): Drop
> >         declarations and blocks only after inlining.  Check that
> >         non-empty blocks are not dropped.
> >         * tree-inline.c (expand_call_inline): Check that functions are
> >         not inlined too late.
>
> > there is a 50% increase in memory consumption and a 5% increase in
> > compile-time for tramp3d.
>
> For the record, two points:
>
> 1. this patch prevents non-empty blocks with referenced variables from
> being dropped.  This was a bug, and it may have given the impression
> that the memory reductions achieved through the earlier, buggy patch
> that IIRC went in on July 24 was correct.  But dropping too much
> information was a mistake.  We could decide it's ok and desirable to
> drop such information, but it has to be a conscious decision, not an
> accidental fallout from a bug.

I agree here.  I am currently evaluating what just reverting the following ...

> 2. this patch delays the garbage-collection of some declarations after
> inlining, to preserve decl UIDs in sync with and without debug
> information.  I personally think that keeping decl UIDs is sync is a
> very desirable property, and it's been quite useful for my recent
> debugging efforts, but there are other ways to do this than keeping
> declarations around just so that they can be cloned at the right time.
> This particular fix should not make any difference if you're compiling
> with -g, except where bug 1. incorrectly dropped entire blocks.

... part of the patch does to the memory usage regression.  This part doesn't
fix a regression and a better implementation strathegy for fixing the underlying
problem should be used IMHO.  To recap, the problem is that while using
UIDs for decls gives you a unique identifier, iterating over a hashtable where
you hash on UIDs doesn't give you a stable order (but it depends on the size
of the hashtable and so for example changes once that is re-allocated).  For
one commonly used hashtable (the referenced_vars hashtable) I have posted
patches that I consider only appropriate for stage1.

Richard.

Re: Put scope blocks on a diet

[Jan Hubicka]

> Alexandre Oliva wrote:
> 
> >> there is a 50% increase in memory consumption and a 5% increase in
> >> compile-time for tramp3d.
> 
> > For the record, two points:
> 
> Alexandre, please take the rhetoric down a notch.  I know you have
> strong feelings about this, but you're among friends. :-)
> 
> > 1. this patch prevents non-empty blocks with referenced variables from
> > being dropped.  This was a bug, and it may have given the impression
> > that the memory reductions achieved through the earlier, buggy patch
> > that IIRC went in on July 24 was correct.  But dropping too much
> > information was a mistake.  We could decide it's ok and desirable to
> > drop such information, but it has to be a conscious decision, not an
> > accidental fallout from a bug.
> 
> That certainly makes sense.  Do you have a URL for the July 24 patch?
> 
> Meanwhile, please revert your patch.  We don't want to oscillate too
> much.  Is there a PR for the problem that your patch fixed?  If not,
> please sure that there is one.

As original author of the diet path, I must say I am with Alexandre
here.  The patch to reduce memory usage on scope blocks was based on
some analysis of memory usage and it was shooting basically for removing
unneded blocks (empty/unreachable) with and without -g.  I added more
aggressive code to remove almost all scope blocks for compilation
without debugging since it seemed like cheap way to get that 50% memory
usage saving on tramp3d.

All earlier GCC releases would use more memory to represent debug info
than current mainline with Alexandre's patch so technically we are not
in regression land here.  Mainline GCC we still take care to remove
empty blocks, blocks that became unreachable and all block after
inlining, while earlier GCCs will keep all the data in memory.  We now
inline bit more because of IPA-SSA changes that is making some
difference, but it is not an uncontrolled explossion.

I agree with Alexandre that it is a bug to produce different code with
and without -g. I didn't originally think this side effect (that
dropping blocks would make inliner not produce duplicated DECLs that
will change DECL_UIDs that will make hashtables to diverge) and if I did
I would not do the optimization.

Of course we don't need all that memory to represent info that will end
up pretty small in DWARF file.  We ought to restructure the info so we
don't duplicate all the DECLs with each inline just to add simple
pointer to DWARF that given decl is duplicated.  I think inliner can
just copy the live declarations that needs to be output fully (ie have
registers assigned) and the "dead" declarations can be just looked up
via ABSTRACT_ORIGIN pointer of the scope block.  This is however might
be bit tricky to get right (I don't feel very confided in debug info
area).

I also believe that 50% of memory usage is extreme case, I know of no
other testcase (can construct artifical one easilly however), where we
would have similar difference.

Honza
> 
> > I don't think letting -g or -g0 affect the executable code output by
> > the compiler is acceptable.
> 
> I don't either.  But, we don't have agreement on how best to solve the
> problem.  So, let's a get PR open, which will help ensure we solve the
> problem before the release, and then let's talk about how we can solve it.
> 
> Thanks,
> 
> -- 
> Mark Mitchell
> CodeSourcery
> mark@codesourcery.com
> (650) 331-3385 x713

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> I agree here.  I am currently evaluating what just reverting the following ...

>> 2. this patch delays the garbage-collection of some declarations after
>> inlining, 

> ... part of the patch does to the memory usage regression.

If you're compiling with -g, it won't do anything at all, for the
!cfun->after_inlining is never even reached when debug_info_level >=
DINFO_LEVEL_NORMAL.

Also, please don't call it a memory usage regression.  It's offensive.
If a patch unintentionally dropped entire basic blocks went in and
remained unfixed for months, would the fix that stops them from being
dropped be regarded as a regression in memory usage?  This is
ridiculous!

Let's please revert the patch that introduced the bug in the first
place.  Then we can make an assessment of how much the memory
optimization patch actually gets us without anyone being confused by
the distortions caused by the bug in the patch.

> This part doesn't fix a regression

I don't understand how you got this mistaken impression.  Without this
part of the patch, or something equivalent, the regression in the
property that -g and -g0 emit the same code, introduced by Honza's
initial patch, remains.  This fundamental property held before his
patch, and it stopped holding afterwards.

Do we consciously want to break this for GCC 4.3?

Also, considering how much scope information that patch causes us to
unintentionally discard, do we consciously want to introduce such a
major debug information regression in GCC 4.3, even with -O0?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 27, 2007 11:56 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>
> > I agree here.  I am currently evaluating what just reverting the following ...
>
> >> 2. this patch delays the garbage-collection of some declarations after
> >> inlining,
>
> > ... part of the patch does to the memory usage regression.
>
> If you're compiling with -g, it won't do anything at all, for the
> !cfun->after_inlining is never even reached when debug_info_level >=
> DINFO_LEVEL_NORMAL.

I am talking about non-debug builds.  Yes, debug builds take a lot of
memory for tramp3d - a different issue that is worth addressing.

> Also, please don't call it a memory usage regression.  It's offensive.
> If a patch unintentionally dropped entire basic blocks went in and
> remained unfixed for months, would the fix that stops them from being
> dropped be regarded as a regression in memory usage?  This is
> ridiculous!

We would have noticed such a patch because we have a quite
extensive testsuite which tests what we seem to care most about.  You
could call it unfortunate that there is no test for the debuginfo part
that Honzas patch broke.  I could call it co-incidence.

> Let's please revert the patch that introduced the bug in the first
> place.  Then we can make an assessment of how much the memory
> optimization patch actually gets us without anyone being confused by
> the distortions caused by the bug in the patch.

?  So you mean comparing the current state with the !inline_completed
part removed to a version that doesn't remove unused scope blocks at all?
That is, what remains in memory savings?

> > This part doesn't fix a regression
>
> I don't understand how you got this mistaken impression.  Without this
> part of the patch, or something equivalent, the regression in the
> property that -g and -g0 emit the same code, introduced by Honza's
> initial patch, remains.  This fundamental property held before his
> patch, and it stopped holding afterwards.
>
> Do we consciously want to break this for GCC 4.3?

This particular part of the patch is just a workaround, and as you don't
have a testcase that breaks (apart from make bootstrap-debug) I cannot
qualify the importance of this regression.  Nevertheless memory-usage
for non-debug builds of applications are an existing issue.

I have a technically more sound patch (those were already posted as
queued for 4.4), and I am going to see if they fix the make bootstra-debug
regression.  So if you consider that regression important enough I'd rather
go with a real fix than a workaround with the memory penalty.  But let
me do the math and verification first.

> Also, considering how much scope information that patch causes us to
> unintentionally discard, do we consciously want to introduce such a
> major debug information regression in GCC 4.3, even with -O0?

I am not proposing to revert this part of the patch.  Please don't repeatedly
say so.

Richard.

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

> Alexandre Oliva wrote:
>>> there is a 50% increase in memory consumption and a 5% increase in
>>> compile-time for tramp3d.

>> For the record, two points:

> Alexandre, please take the rhetoric down a notch.  I know you have
> strong feelings about this, but you're among friends. :-)

I thought you agreed that -g and -g0 ought to emit the same executable
code.  Did you change your mind about this?  Seriously!

As if it wasn't enough to spend 3 months trying to get a major
regression in this property fixed, and doing that out of shared
consideration for the effort to control memory use, I now have to
revert the fix and sacrifice this property for the sake of preserving
a *bug* that *largely exaggerated* the memory reduction of the
original broken patch?

> That certainly makes sense.  Do you have a URL for the July 24 patch?

http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01745.html

> Meanwhile, please revert your patch.

Can you justify the double standards being applied here?

One major bug remains unfixed for months and the patch that introduced
it doesn't get reverted.

A patch that fixes it needs immediate reversion?

This is insulting.  Please at least make an effort to understand where
the problem actually lies, and to give any indication that you did,
before coming to a decision on this.  The decision ATM is prejudicial
and absurd.

I'd happily revert both patches.  But reverting to the broken state we
were in before the fix just gets people confused as to understanding
the effects of the patch.

> We don't want to oscillate too much.

Then how about we avoid yet another oscillation and figure out what to
do before reverting to the broken state we had before?

> Is there a PR for the problem that your patch fixed?  If not,
> please sure that there is one.

I'll open one right away.

>> I don't think letting -g or -g0 affect the executable code output by
>> the compiler is acceptable.

> I don't either.  But, we don't have agreement on how best to solve the
> problem.

It could be as easy as reverting the patch that caused it, no?  If
that "works" for the bug fix, it ought to work even more immediately
for the patch that introduced the bug, no?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 28, 2007 12:11 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 27, 2007, Mark Mitchell <mark@codesourcery.com> wrote:
>
> > That certainly makes sense.  Do you have a URL for the July 24 patch?
>
> http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01745.html
>
> > Meanwhile, please revert your patch.
>
> Can you justify the double standards being applied here?

Please leave the patch in for now (even if I disagree about the
!inline_completed
part).  I'll have some additional numbers by tomorrow.

Thanks
Richard.

Re: Put scope blocks on a diet

[Mark Mitchell]

Alexandre Oliva wrote:

>> Alexandre, please take the rhetoric down a notch.  I know you have
>> strong feelings about this, but you're among friends. :-)
> 
> I thought you agreed that -g and -g0 ought to emit the same executable
> code.  Did you change your mind about this?  Seriously!

No, and I said so explicitly in the message to which you are responding.
 Please note that I specifically asked you to open a PR so that we could
mark it as important to fix before any future release.  That was meant
as a clear signal that I heard your concern and believed it had merit.

I don't have a "double standard", etc.  I have no goals here except that
GCC be as good as possible.  I'm doing my best to mediate between
competing interests here and it's not always easy.  I certainly make
plenty of mistakes, and perhaps this is one of them.  If I knew I was
making a mistake before I did it, I'd stop. :-)

Disagreements occur sometimes.  This is all going to be fine.  It may
just take a little bit of time to work it out.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

>> Let's please revert the patch that introduced the bug in the first
>> place.  Then we can make an assessment of how much the memory
>> optimization patch actually gets us without anyone being confused by
>> the distortions caused by the bug in the patch.

> ?  So you mean comparing the current state with the !inline_completed
> part removed to a version that doesn't remove unused scope blocks at all?
> That is, what remains in memory savings?

Yes.  Not being fooled by the excessive memory savings caused by the
bug.

> This particular part of the patch is just a workaround, and as you don't
> have a testcase that breaks (apart from make bootstrap-debug)

On what grounds do you disregard maek bootstrap-debug as a testcase?
Do you know of any other mechanisms we have to detect changes in
generated code caused by enabling debug information?

> Nevertheless memory-usage for non-debug builds of applications are
> an existing issue.

I never said it wasn't.  I just don't think it justifies breaking the
-g/-g0 principle.

> I have a technically more sound patch (those were already posted as
> queued for 4.4), and I am going to see if they fix the make bootstra-debug
> regression.

Is it going to cause decl uids to vary with or without debug info?
This would be very unfortunate and IMHO highly undesirable.

> So if you consider that regression important enough I'd rather
> go with a real fix than a workaround with the memory penalty.

That's fine with me.  I'm not opposed to memory savings, obviously.
What I'm opposed to is sacrificing fundamental requirements for the
sake of memory savings.

>> Also, considering how much scope information that patch causes us to
>> unintentionally discard, do we consciously want to introduce such a
>> major debug information regression in GCC 4.3, even with -O0?

> I am not proposing to revert this part of the patch.

I didn't see anyone saying "please revert one part of the patch".  I
saw people panicing and jumping to demand the whole patch to be
reverted, before it was even understood that this would bringing back
the far more fundamental regression the patch was meant to fix.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

> Alexandre Oliva wrote:
>>> Alexandre, please take the rhetoric down a notch.  I know you have
>>> strong feelings about this, but you're among friends. :-)
>> 
>> I thought you agreed that -g and -g0 ought to emit the same executable
>> code.  Did you change your mind about this?  Seriously!

> No, and I said so explicitly in the message to which you are responding.
>  Please note that I specifically asked you to open a PR so that we could
> mark it as important to fix before any future release.  That was meant
> as a clear signal that I heard your concern and believed it had merit.

I believe you, but reverting the patch doesn't support this stance.
If we really want the problem fixed, we ought to keep both patches in,
or revert both patches.  Leaving it longer at the broken state will
just ensure that nobody will fix it.  By keeping it functional and
requiring fixes to keep it functional we place the incentives at the
right spot.

> I don't have a "double standard", etc.

Then how do you explain the decision to revert the patch that fixed a
bug without reverting the patch that caused the bug?

What's the rush to revert a fix for a known and serious bug?

What's the point of rushing to do it before the problem is fully
understood?  It's not like the next patch to fix the problem in a
different way can't include the reversion of the relevant bits.  It's
not like we want to keep the tree broken longer.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

>  Please note that I specifically asked you to open a PR 

Rats, two sent mails that I started in order to post the PR number,
and twice have I forgotten to do it :-(

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34255

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Mark Mitchell]

Alexandre Oliva wrote:

>> I don't have a "double standard", etc.
> 
> Then how do you explain the decision to revert the patch that fixed a
> bug without reverting the patch that caused the bug?

I made the decision that seemed best to me at the time, based on the
information I had at the time.  One of my duties is to pick least-bad
alternatives in situations where we have no clear win.  I am happy to
have my decisions criticized, but you seem to be accusing me of some
malice, malfeasance, or carelessness and I don't think that's appropriate.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 28, 2007 1:39 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 27, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>
> >> Let's please revert the patch that introduced the bug in the first
> >> place.  Then we can make an assessment of how much the memory
> >> optimization patch actually gets us without anyone being confused by
> >> the distortions caused by the bug in the patch.
>
> > ?  So you mean comparing the current state with the !inline_completed
> > part removed to a version that doesn't remove unused scope blocks at all?
> > That is, what remains in memory savings?
>
> Yes.  Not being fooled by the excessive memory savings caused by the
> bug.

Ok, so I have numbers with

Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c     (revision 130489)
+++ tree-ssa-live.c     (working copy)
@@ -501,11 +501,7 @@ remove_unused_scope_block_p (tree scope)
         only the used variables for cfgexpand's memory packing saving quite
         a lot of memory.  */
       else if (debug_info_level == DINFO_LEVEL_NORMAL
-              || debug_info_level == DINFO_LEVEL_VERBOSE
-              /* Removing declarations before inlining is going to affect
-                 DECL_UID that in turn is going to affect hashtables and
-                 code generation.  */
-              || !cfun->after_inlining)
+              || debug_info_level == DINFO_LEVEL_VERBOSE)
        unused = false;

       else

on top of current trunk (which includes your patch).  And (no surprise here)
the regression is completely gone (remember, this is a non-debug build).

> > This particular part of the patch is just a workaround, and as you don't
> > have a testcase that breaks (apart from make bootstrap-debug)
>
> On what grounds do you disregard maek bootstrap-debug as a testcase?
> Do you know of any other mechanisms we have to detect changes in
> generated code caused by enabling debug information?

Well, a testcase is something that can be checked in the testsuite to
see if it regresses (I admit that -g vs. -g0 mismatches are very sensitive
to changes anywhere in the compiler and come and go once you start
reducing a testcase).

> > Nevertheless memory-usage for non-debug builds of applications are
> > an existing issue.
>
> I never said it wasn't.  I just don't think it justifies breaking the
> -g/-g0 principle.

Well, if we want to make that priciple "absolut", the only sane way to
implement it is to just always follow the same path during compilation
(that is, do as if -g was enabled) and only at the point of debug output
do nothing.  _That_ would fix it for sure - and would be more maintainable
as well.  Just - it might cause memory-usage and compile-time regressions
for formerly non-debug builds.

I don't see us taking the -g/-g0 principle to that level unless creating
debug information is _way_ cheaper than now.  (If the regression
for non-debug would be minor then I would be all for it)

> > I have a technically more sound patch (those were already posted as
> > queued for 4.4), and I am going to see if they fix the make bootstra-debug
> > regression.
>
> Is it going to cause decl uids to vary with or without debug info?
> This would be very unfortunate and IMHO highly undesirable.

Yes and no.  Code generated shouldn't depend on varying UIDs.  In
principle bootstrap comparison should survive even if we did

    DECL_UID (t) = next_decl_uid;
    next_decl_uid += random() % 13;

anything else I consider a bug in how we use DECL_UIDs.

That the DECL_UIDs printed in the dumps would be different with/without
debug info is unfortunate, but you can make it less worse by making some
room for extra UIDs per function (that is, "rounding" to the next 10000
before emitting the next function).

But surely you now won't make consistent DECL_UIDs in debug dump
-g/-g0 a correctness issue?

> > So if you consider that regression important enough I'd rather
> > go with a real fix than a workaround with the memory penalty.
>
> That's fine with me.  I'm not opposed to memory savings, obviously.
> What I'm opposed to is sacrificing fundamental requirements for the
> sake of memory savings.

If it is that fundamental the only way you can prove that code isn't different
-g vs. -g0 is to always go the debug path during compilation.  Anything else
doesn't make sense - or the issue isn't as black-and-white as you suggest.

> >> Also, considering how much scope information that patch causes us to
> >> unintentionally discard, do we consciously want to introduce such a
> >> major debug information regression in GCC 4.3, even with -O0?
>
> > I am not proposing to revert this part of the patch.
>
> I didn't see anyone saying "please revert one part of the patch".  I
> saw people panicing and jumping to demand the whole patch to be
> reverted, before it was even understood that this would bringing back
> the far more fundamental regression the patch was meant to fix.

Well, you disregard the discussion we had on IRC and you disregard
mails sent after the inital request to revert (the whole) patch.

I'll now try that make bootstrap-debug.

Richard.

Re: Put scope blocks on a diet

[Mark Mitchell]

Richard Guenther wrote:

>> I never said it wasn't.  I just don't think it justifies breaking the
>> -g/-g0 principle.
> 
> Well, if we want to make that priciple "absolut"

To make sure we're all talking about the same thing, the principle I'm
referring to is that the instruction sequence generated for "-O2" and
"-g -O2" be identical, or, more generally, that adding or removing "-g"
to any command line not change the instruction sequence.

That principle *is* absolute.  It has been for a long time.  I remember
fixing bugs like this many years ago.  (A frequent cause used to be not
remembering to skip NOTEs in the RTL stream, which would only be present
with debugging turned on.)  This is an important promise that we make to
users so that they can turn on -g to help figure out what's wrong with
optimized code.

> the only sane way to
> implement it is to just always follow the same path during compilation
> (that is, do as if -g was enabled) and only at the point of debug output
> do nothing.

That would certainly fix it, but I don't think it's the only feasible
way.  Modulo the occasional bug, we've been able to make this work for a
long time without doing this.  I agree that if your solution were the
only feasible approach we might have to make a tough decision (because
of the cost of building up debug information), but, fortunately, I don't
think that's the only feasible approach, so I don't think we have to
make that choice.

> Yes and no.  Code generated shouldn't depend on varying UIDs.  In
> principle bootstrap comparison should survive even if we did
> 
>     DECL_UID (t) = next_decl_uid;
>     next_decl_uid += random() % 13;
> 
> anything else I consider a bug in how we use DECL_UIDs.

I don't think that's been true historically.  IIRC, we've considered
DECL_UIDs to be acceptable for use as hash/sort keys, even though we
didn't want to use pointers.  (Pointers, of course, can differ from run
to run with address-space randomization -- but, in the old days, the
biggest problem was that they differed across hosts.)  I think that
means that, in the past, DECL_UIDs must been the same between  -O2 and
-g -O2 builds.

We could weaken our assumptions, as you suggest, so that only the
relative ordering of DECL_UIDs matters.  And, if that allows us to save
a lot of memory, then that might be a better solution than trying to
make DECL_UIDs the same between debug and non-debug builds.

Can we find the places that are walking the hash table and have them
collect the declarations first, sort them by DECL_UID, and then walk the
sorted array?  Or, use a splay tree instead of a hash table, since splay
trees already have the property that walking the tree will always go in
sorted order?  (Splay trees are of course O(log n) for most operations,
whereas hashtables are O(1), ignoring resizing costs.)

> But surely you now won't make consistent DECL_UIDs in debug dump
> -g/-g0 a correctness issue?

Indeed not.  The content of the dumps isn't important, except in that it
helps us debug the compiler.  But, it is very important that "-g -O2"
and "-g" generate bit-for-bit identical instruction sequences.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 28, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> on top of current trunk (which includes your patch).  And (no surprise here)
> the regression is completely gone (remember, this is a non-debug build).

Thanks for checking.  Just to be clear, you're talking about the
memory-reduction regression, not the -g-mustn't-change-code
regression, right?

>> On what grounds do you disregard maek bootstrap-debug as a testcase?
>> Do you know of any other mechanisms we have to detect changes in
>> generated code caused by enabling debug information?

> Well, a testcase is something that can be checked in the testsuite to
> see if it regresses

The test process for any patch involves a bootstrap before the
testsuite is run.  Now that we have infrastructure to test for debug
info, I'd like to see that changed to bootstrap-debug.  It's faster
and it tests this property, with at least as much coverage as a GCC
bootstrap gives us (which is admittedly incomplete).

>> I just don't think it justifies breaking the -g/-g0 principle.

> Well, if we want to make that priciple "absolut", the only sane way to
> implement it is to just always follow the same path during compilation

That's a wasteful way to do it.  The other way to pursue this goal is
to be more efficient when possible, and be watchful for divergencies,
fixing them without hesitation when they show up.  Very much like we'd
do for executable code generation bugs.  In fact, I can see a similar
version of your argument stating that the only sane way to write a
compiler is to not perform any optimizations whatsoever, since every
such transformation can modify the behavior of the program.  But this
nonsensical conclusion shows that there's something wrong with the
premise.

Sure, avoiding bugs and fixing them as soon as possible after they're
discovered is more work, but it's a good trade-off given that we want
an optimizing compiler capable of emitting both executable code and
debug information correctly, and that doesn't modify the executable
code depending on whether debug information is being emitted, for
practical reasons that justified an early design principle that has so
far been unchallenged.

But, just like we use testing to make sure that optimizations don't
break properties we want to maintain in executable code, we can use
testing to make sure that debug information generation doesn't.  Enter
bootstrap-debug.  And the nice thing is that it doesn't only exercise
more paths in the compiler, it is also faster than a full bootstrap
with debug information enabled (the current default).  We could easily
suggest it as the default testing procedure, at least on platforms on
which it works, for I honestly don't know how portable bit-per-bit
comparison of stripped objects is.

Along the same lines, there's the still-ugly collection of 3 patches I
proposed a few days ago, that introduces means to bootstrap-compare
the whole tree, rather than just the gcc sub-directory, for bootstrap
miscompares, and that introduces means to easily perform yet another
build of the target libraries with -g0, to compare with the -g build
and extend the coverage of testing of the -g/-g0 property to other
languages that exercise paths outside those exercised by the portable
subset of C and Ada that our front-, middle- and back-ends rely on.

With this patchset, I could verify that the only difference between -g
and -g0 ATM, in the VTA branch (that has -fvar-tracking-assignments
enabled by default if -g is enabled) on x86_64-linux-gnu is in
gcc/ada/rts/sysdep.o, and it's a nearly-irrelevant one: with -g, some
global static pointers to strings that are unused and thus optimized
away have their initializer strings emitted in non-debug sections, but
they're not emitted at all without -g.

>> Is it going to cause decl uids to vary with or without debug info?
>> This would be very unfortunate and IMHO highly undesirable.

> Yes and no.  Code generated shouldn't depend on varying UIDs.

Do you include debug information under "code generated"?  I honestly
don't know whether decl uids, used as identifiers for purposes of
debug dumps, aren't used in debug information too.  I guess not, but I
haven't verified, and I'm wondering if you did.

> In principle bootstrap comparison should survive even if we did

>     DECL_UID (t) = next_decl_uid;
>     next_decl_uid += random() % 13;

> anything else I consider a bug in how we use DECL_UIDs.

Agreed.

> That the DECL_UIDs printed in the dumps would be different with/without
> debug info is unfortunate, but you can make it less worse by making some
> room for extra UIDs per function (that is, "rounding" to the next 10000
> before emitting the next function).

That helps, for sure, but it makes debugging more painful, because
"next function" is a very fuzzy notion for inlining passes, and for
"time-shared" execution of passes that create new decls for different
functions.  (by time-shared, I mean running some passes for some
functions, then running some passes for other functions, then going
back to the first functions, and even doing so in a different order)

And then, even if you round before each declaration-generating pass,
it would become far more difficult to track divergencies across
inlining.

I don't think keeping DECL_UIDs is much more useful to debugging any
kind of problem other than -g/-g0 divergencies, so this desirable
property shouldn't trample other more important properties, such as
saving memory or making the compiler faster.  But I would like to
achieve similar improvements without destroying this property and
without creating maintainability problems, and I believe this is
possible and practical.

> But surely you now won't make consistent DECL_UIDs in debug dump
> -g/-g0 a correctness issue?

Heh.  No, certainly not.  That's why I chose the words "unfortunate"
and "undesirable", rather than "wrong".

This property is not a fundamental design decision, and we don't have
agreement (or even a proposal thereof) that it's important enough to
be elevated to a strict requirement.  It's merely desirable.  That it
has the nice side effect that we can then use uids for hashing, when
we know the number of entries is stable, is just a bonus, even if a
bonus of debatable value ;-) But we could live without it, for sure.
Even nicer would be some option to *make* decl uids stable, at some
memory penalty.  I.e., some debug-dump option (with similar purpose as
-fdump-unnumbered, that makes rtl dumps easier to compare) that could
be tested along with !cfun->after_inlining at the spot you proposed to
modify, so as to *optionally* keep DECL_UID in sync, thus making TREE
dumps easier to compare.

> Well, you disregard the discussion we had on IRC and you disregard
> mails sent after the inital request to revert (the whole) patch.

I don't disregard them.  By the time we got past it, the damage was
already done.

The request and decision to revert the patch before gathering or
requesting information or giving someone who understood the issue a
chance to explain it was harmful to a collaborative development
community.

I certainly didn't help with my heated reaction, for which I
apologize, but I hope you and Mark can understand how insulted I felt
for not having been consulted before the request and the decision.  It
certainly didn't help that we have an ongoing heated and painful
debate on an issue in which similar terms show up, to the point that
even we have had trouble realizing that this patch was about a
completely different issue.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 28, 2007 7:48 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Richard Guenther wrote:
>
> >> I never said it wasn't.  I just don't think it justifies breaking the
> >> -g/-g0 principle.
> >
> > Well, if we want to make that priciple "absolut"
>
> To make sure we're all talking about the same thing, the principle I'm
> referring to is that the instruction sequence generated for "-O2" and
> "-g -O2" be identical, or, more generally, that adding or removing "-g"
> to any command line not change the instruction sequence.
>
> That principle *is* absolute.  It has been for a long time.  I remember
> fixing bugs like this many years ago.  (A frequent cause used to be not
> remembering to skip NOTEs in the RTL stream, which would only be present
> with debugging turned on.)  This is an important promise that we make to
> users so that they can turn on -g to help figure out what's wrong with
> optimized code.

Right, I agree here.

> > the only sane way to
> > implement it is to just always follow the same path during compilation
> > (that is, do as if -g was enabled) and only at the point of debug output
> > do nothing.
>
> That would certainly fix it, but I don't think it's the only feasible
> way.  Modulo the occasional bug, we've been able to make this work for a
> long time without doing this.  I agree that if your solution were the
> only feasible approach we might have to make a tough decision (because
> of the cost of building up debug information), but, fortunately, I don't
> think that's the only feasible approach, so I don't think we have to
> make that choice.

The difference I want to make here is whether we want to accumulate fixes
(or hacks, dependent on how you see them) or if we really want to analyze
what causes the divergence.  At some point it may be better for maintainance
reasons to not make -g vs -g0 different (apart from output).

> > Yes and no.  Code generated shouldn't depend on varying UIDs.  In
> > principle bootstrap comparison should survive even if we did
> >
> >     DECL_UID (t) = next_decl_uid;
> >     next_decl_uid += random() % 13;
> >
> > anything else I consider a bug in how we use DECL_UIDs.
>
> I don't think that's been true historically.

Historically?  Ok, I'm new enough to gcc to not remember ;)

> IIRC, we've considered
> DECL_UIDs to be acceptable for use as hash/sort keys, even though we
> didn't want to use pointers.  (Pointers, of course, can differ from run
> to run with address-space randomization -- but, in the old days, the
> biggest problem was that they differed across hosts.)  I think that
> means that, in the past, DECL_UIDs must been the same between  -O2 and
> -g -O2 builds.

As far as I would say we use DECL_UIDs because you can establish an
ordering between UIDs while you cannot do so with pointers.  If you hash
either of them to implement set or map datastructures they are not different.
The difference is in how you can iterate in a reproducible way over a
set of them.  With UIDs because you have a ordering, you can iterate in
a defined ordering.  With pointers this is not possible.  So if you have an
algorithm whose result depends on the order of its inputs, then you better
define the ordering you prefer - which you can only fulfil if you have input
that can be sorted in any way.  Thus we use UIDs.  Now if we iterate over
a _hashtable_ (even if we hashed UIDs), your hashtable element ordering
does not map to a stable ordering of UIDs.

> We could weaken our assumptions, as you suggest, so that only the
> relative ordering of DECL_UIDs matters.  And, if that allows us to save
> a lot of memory, then that might be a better solution than trying to
> make DECL_UIDs the same between debug and non-debug builds.

Any ordering of UIDs only depends on "relative" ordering.

> Can we find the places that are walking the hash table and have them
> collect the declarations first, sort them by DECL_UID, and then walk the
> sorted array?  Or, use a splay tree instead of a hash table, since splay
> trees already have the property that walking the tree will always go in
> sorted order?  (Splay trees are of course O(log n) for most operations,
> whereas hashtables are O(1), ignoring resizing costs.)

Yes, I have done so for referenced_vars by using a bitmap indexed by UID.
Which is both space-efficient and fast in average (though O(n) in theory).
But this is not appropriate for stage3.

I think we need to be more careful in how we use UIDs - they are not in
itself a thing that leads to less problems.  And identifying the remaining
problematic cases is work.  Papering over the issue by disabling "optimizations"
for the non-debug case is bad IMHO, unless we want to unify the debug/non-debug
paths in general.

Thanks,
Richard.

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 28, 2007 9:49 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 28, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
>
> > on top of current trunk (which includes your patch).  And (no surprise here)
> > the regression is completely gone (remember, this is a non-debug build).
>
> Thanks for checking.  Just to be clear, you're talking about the
> memory-reduction regression, not the -g-mustn't-change-code
> regression, right?

Yes.  The -g-musn't-change-code regression (as of make bootstrap-debug failing)
is still there if this part of the patch is reverted.

> >> On what grounds do you disregard maek bootstrap-debug as a testcase?
> >> Do you know of any other mechanisms we have to detect changes in
> >> generated code caused by enabling debug information?
>
> > Well, a testcase is something that can be checked in the testsuite to
> > see if it regresses
>
> The test process for any patch involves a bootstrap before the
> testsuite is run.  Now that we have infrastructure to test for debug
> info, I'd like to see that changed to bootstrap-debug.  It's faster
> and it tests this property, with at least as much coverage as a GCC
> bootstrap gives us (which is admittedly incomplete).
>
> >> I just don't think it justifies breaking the -g/-g0 principle.
>
> > Well, if we want to make that priciple "absolut", the only sane way to
> > implement it is to just always follow the same path during compilation
>
> That's a wasteful way to do it.  The other way to pursue this goal is
> to be more efficient when possible, and be watchful for divergencies,
> fixing them without hesitation when they show up.  Very much like we'd
> do for executable code generation bugs.  In fact, I can see a similar
> version of your argument stating that the only sane way to write a
> compiler is to not perform any optimizations whatsoever, since every
> such transformation can modify the behavior of the program.  But this
> nonsensical conclusion shows that there's something wrong with the
> premise.

It depends.  We may come to the point that enabling -g doesn't consume
measurably more ressources.  Which would be indeed nice.

> Sure, avoiding bugs and fixing them as soon as possible after they're
> discovered is more work, but it's a good trade-off given that we want
> an optimizing compiler capable of emitting both executable code and
> debug information correctly, and that doesn't modify the executable
> code depending on whether debug information is being emitted, for
> practical reasons that justified an early design principle that has so
> far been unchallenged.

That is absolutely a goal we should try to fulfil.  Though it may not matter
that much in practice as we are either good enough, or in cases where
it matters you build with -g, ship stripped binaries and have the debug
information extracted from the build and saved in -debuginfo packages.

> But, just like we use testing to make sure that optimizations don't
> break properties we want to maintain in executable code, we can use
> testing to make sure that debug information generation doesn't.  Enter
> bootstrap-debug.  And the nice thing is that it doesn't only exercise
> more paths in the compiler, it is also faster than a full bootstrap
> with debug information enabled (the current default).  We could easily
> suggest it as the default testing procedure, at least on platforms on
> which it works, for I honestly don't know how portable bit-per-bit
> comparison of stripped objects is.

Well, if we want not to break the -g vs. -g0 property then we should
enable this by default for regular bootstrap (maybe somehow distinguish
debug comparison failures from code comparison failures).

> > That the DECL_UIDs printed in the dumps would be different with/without
> > debug info is unfortunate, but you can make it less worse by making some
> > room for extra UIDs per function (that is, "rounding" to the next 10000
> > before emitting the next function).
>
> That helps, for sure, but it makes debugging more painful, because
> "next function" is a very fuzzy notion for inlining passes, and for
> "time-shared" execution of passes that create new decls for different
> functions.  (by time-shared, I mean running some passes for some
> functions, then running some passes for other functions, then going
> back to the first functions, and even doing so in a different order)
>
> And then, even if you round before each declaration-generating pass,
> it would become far more difficult to track divergencies across
> inlining.
>
> I don't think keeping DECL_UIDs is much more useful to debugging any
> kind of problem other than -g/-g0 divergencies, so this desirable
> property shouldn't trample other more important properties, such as
> saving memory or making the compiler faster.  But I would like to
> achieve similar improvements without destroying this property and
> without creating maintainability problems, and I believe this is
> possible and practical.

Sure - I even fixed an off-by-one bug in this area lately.

> > But surely you now won't make consistent DECL_UIDs in debug dump
> > -g/-g0 a correctness issue?
>
> Heh.  No, certainly not.  That's why I chose the words "unfortunate"
> and "undesirable", rather than "wrong".
>
> This property is not a fundamental design decision, and we don't have
> agreement (or even a proposal thereof) that it's important enough to
> be elevated to a strict requirement.  It's merely desirable.  That it
> has the nice side effect that we can then use uids for hashing, when
> we know the number of entries is stable, is just a bonus, even if a
> bonus of debatable value ;-) But we could live without it, for sure.
> Even nicer would be some option to *make* decl uids stable, at some
> memory penalty.  I.e., some debug-dump option (with similar purpose as
> -fdump-unnumbered, that makes rtl dumps easier to compare) that could
> be tested along with !cfun->after_inlining at the spot you proposed to
> modify, so as to *optionally* keep DECL_UID in sync, thus making TREE
> dumps easier to compare.

Just to compare -g vs. -g0 dumps?  I don't think I ever needed to do that
during day-to-day work.  But yes, less divergence in the dumps would be nice,
and you can produce that with simple scripts that rip out UIDs of temporaries.

Richard.

Re: Put scope blocks on a diet

[Mark Mitchell]

Richard Guenther wrote:

> The difference I want to make here is whether we want to accumulate fixes
> (or hacks, dependent on how you see them) or if we really want to analyze
> what causes the divergence.  At some point it may be better for maintainance
> reasons to not make -g vs -g0 different (apart from output).

Right, a priori, either approach seems plausible.  My guess is that in
practice the cost of accumulating all the debug information only to
throw it away is going to be too expensive, but I've been wrong plenty
of times before.

> As far as I would say we use DECL_UIDs because you can establish an
> ordering between UIDs while you cannot do so with pointers.

I think that's a good observation, but my impression was that the
motivation was actually that DECL_UIDs are consistent across hosts and
across runs.  In any case, both properties are useful.

> Now if we iterate over
> a _hashtable_ (even if we hashed UIDs), your hashtable element ordering
> does not map to a stable ordering of UIDs.

True -- but it is consistent across runs and across hosts.  Unless the
UIDs vary when you add -g, which, empirically, they do.  I don't know if
that was always true (and we never noticed) or if this is a new
property.  (I've gotten a little lost on the actual technical details here.)

>> Can we find the places that are walking the hash table and have them
>> collect the declarations first, sort them by DECL_UID, and then walk the
>> sorted array?  Or, use a splay tree instead of a hash table, since splay
>> trees already have the property that walking the tree will always go in
>> sorted order?  (Splay trees are of course O(log n) for most operations,
>> whereas hashtables are O(1), ignoring resizing costs.)
> 
> Yes, I have done so for referenced_vars by using a bitmap indexed by UID.
> Which is both space-efficient and fast in average (though O(n) in theory).
> But this is not appropriate for stage3.

Perhaps not, but other fixes might be.  If that's the problematic table,
then replacing the walk function with a walk (to collect the nodes), a
qsort, and an iteration might be fine, and very contained.

> I think we need to be more careful in how we use UIDs - they are not in
> itself a thing that leads to less problems.  And identifying the remaining
> problematic cases is work.  Papering over the issue by disabling "optimizations"
> for the non-debug case is bad IMHO, unless we want to unify the debug/non-debug
> paths in general.

I agree that we absolutely do not want to disable code-generation
optimizations for the non-debug case.  We also don't want to disable
compile-time optimizations, if we can help it, but we should be willing
to pay a little bit -- and no more -- of compile-time if absolutely
necessary.  (No, I don't have a concrete bounding of "a little bit"...)

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 28, 2007 11:01 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Richard Guenther wrote:
>
> >> Can we find the places that are walking the hash table and have them
> >> collect the declarations first, sort them by DECL_UID, and then walk the
> >> sorted array?  Or, use a splay tree instead of a hash table, since splay
> >> trees already have the property that walking the tree will always go in
> >> sorted order?  (Splay trees are of course O(log n) for most operations,
> >> whereas hashtables are O(1), ignoring resizing costs.)
> >
> > Yes, I have done so for referenced_vars by using a bitmap indexed by UID.
> > Which is both space-efficient and fast in average (though O(n) in theory).
> > But this is not appropriate for stage3.
>
> Perhaps not, but other fixes might be.  If that's the problematic table,
> then replacing the walk function with a walk (to collect the nodes), a
> qsort, and an iteration might be fine, and very contained.

Yes, unfortunately I have not identified the walk that causes the problem (where
there might be more than one), but chose to exchange the underlying
data-structure
(also because I needed the global lookup table anyway) which fixes all of the
FOR_EACH_REFERENCED_VAR users - which are quite a few.  For reference, the
two patches are

Introduce a global DECL_UID to tree map:
http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00523.html

Make gimple_df->referenced_vars a bitmap indexed by UID rather than a hashtable:
http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00524.html

In the end these patches should enable us to get rid of the unexpanded_var_list
(and thus the use of TREE_CHAIN for decls) and of the remove_unused_vars
TODO (which takes up quite an amount of time for tramp3d) and let the garbage
collector do this work.

Richard.

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 28, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

> True -- but it is consistent across runs and across hosts.  Unless the
> UIDs vary when you add -g, which, empirically, they do.

For the record, before the July 2[46] patch, they didn't.  After that
patch, they did.  And, ever since I started caring deeply about them
(when I started the VTA project), that was the only regression and the
only detail I had to fix to keep DECL_UIDs in sync comparing -g and
-g0.

My suggestion is that, when not emitting debug information, instead of
keeping unused decls around in the binding blocks, we replace them
with placeholders, such that, at inlining time, instead of cloning
the declarations they would have represented, we can merely bump up
the decl uid counter.

This *would* keep UIDs in sync with a far less destabilizing effect.
However, I can't tell for sure that none of the removed decls would
have made to hash tables elsewhere, which could potentially
destabilize the compiler output by changing hashtable walk order
because of changes in the hashtable size.  I don't quite *see* how
unused variables would make it elsewhere, and it's simple enough that
it's probably worth a shot.  But I can't promise it's going to work.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 27, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

> Alexandre Oliva wrote:
>>> I don't have a "double standard", etc.
>> 
>> Then how do you explain the decision to revert the patch that fixed a
>> bug without reverting the patch that caused the bug?

> I made the decision that seemed best to me at the time, based on the
> information I had at the time.

As I mentioned in another e-mail (but it was long, so I'll restate
here), I think the worst part of it was not seeking more information
before making the decision, or even the request.  A lot of discussion
on the problem the patch fixed was readily available, and I'd have
been happy to point it out (like I did) without having the additional
worry of disrespecting a decision.

> I am happy to have my decisions criticized, but you seem to be
> accusing me of some malice, malfeasance, or carelessness and I don't
> think that's appropriate.

I apologize for having left my frustration give you such a strong
impression.  My feeling was that my earlier assessments regarding the
problem and the patch had been taken into account and dismissed,
perhaps out of distrust for my skills.

I can live with not having earned enough trust to have my technical
assessments taken at face value; after all, trust for one's judgement
is something that one has to earn, not demand or impose ;-)  For the
record, that was the double-standards that I was challenging, and now
I realize even that was inappropriate, for the reason stated in this
very paragraph.

(I'll not get into the possibility of distrust for my ethics, as in
assuming I lied or something like that; I very much doubt there was
any such assumption)

But I found that having had the decision made before I was even
consulted about it, after I spent so much time understanding,
discussing and working on the problem, was quite inappropriate.

Not only for the implied (even if unintended) insult, but also
because, after I presented and summarized the background for the
patch, the decision was revised.

This leads me to believe that the decision was made without enough
information.  I guess at times such difficult calls have to be made,
even in the absence of enough information.  But for the future I
suggest, as friendly advice for community building (yeah, right, like
I'm an expert on that ;-) some effort to obtain more information from
the patch author, or someone else involved in its development or
approval or otherwise knowledgeable in the specific issue at hand,
before a decision to revert it is announced, or even requested.

A suggestion to revert it, along with some questions about the patch,
would be far more friendly, I think.

Thanks in advance for taking this into account, and, one more time,
please accept my apologies for my inappropriate harshness and
impoliteness.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 28, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> It depends.  We may come to the point that enabling -g doesn't consume
> measurably more ressources.  Which would be indeed nice.

Hey, but that's easy!  We just make sure compiling without -g wastes
lots of resources.  Then, -g won't consume more ;-)


Seriously, there's no other way -g won't consume measurably more
resources.  There's no way you can fit more information in the same
space as less information, unless the less-information case is highly
redundant, which indicates a poor job in packing less information.  If
less information was stored as efficiently as stand-alone as with the
rest of the information that makes up more, then it would certainly
take up less resources.  This is information theory 101.  (Shall we
discuss infinite compression algorithms and perpetual motion engines
now? :-)


In order to generate debug information, it's not enough to keep only
the information needed to output executable code.  You have to keep a
mapping from source concepts to the executable code.  This takes up
resources.  And the more you optimize, the more complex this mapping
becomes, and therefore the more additional resources you need to
represent them, both in the internal representation and in the debug
information output.

> That is absolutely a goal we should try to fulfil.  Though it may not matter
> that much in practice as we are either good enough, or in cases where
> it matters you build with -g, ship stripped binaries and have the debug
> information extracted from the build and saved in -debuginfo packages.

Weren't we having a discussion just now about the importance of saving
memory for builds without -g?  I guess it does matter in practice,
after all ;-)

>> bootstrap-debug.  And the nice thing is that it doesn't only exercise
>> more paths in the compiler, it is also faster than a full bootstrap
>> with debug information enabled (the current default).  We could easily
>> suggest it as the default testing procedure, at least on platforms on
>> which it works, for I honestly don't know how portable bit-per-bit
>> comparison of stripped objects is.

> Well, if we want not to break the -g vs. -g0 property then we should
> enable this by default for regular bootstrap

+1

> (maybe somehow distinguish debug comparison failures from code
> comparison failures).

There's no such thing as debug comparison failures.  bootstrap-debug
doesn't compare debug information.  It strips out debug information
and compares only the actual code.

bootstrap-debug builds stage2 without -g (or, actually, with -g0,
which is like no -g, if someone doesn't have that clear) and stage3
with -g, and makes sure the generated code is the same for both.

> Just to compare -g vs. -g0 dumps?

Yes.  That makes finding out where a bootstrap-debug failure comes
from.

> But yes, less divergence in the dumps would be nice, and you can
> produce that with simple scripts that rip out UIDs of temporaries.

This doesn't help you locate the divergencies where they matter,
because you've thrown away the baby along with the bathtub water.  The
divergencies are precisely where the differences in generated code
have come from, in several of the cases I've debugged over the past
couple of months in the VTA branch.

So, yes, as I pointed out before, the usefulness of this property is
quite limited, but it's also cheap to achieve with the placeholders I
proposed.  It's even cheaper than the additional bitmaps you propose,
my intuition tells me.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 29, 2007 3:06 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>
> > But yes, less divergence in the dumps would be nice, and you can
> > produce that with simple scripts that rip out UIDs of temporaries.
>
> This doesn't help you locate the divergencies where they matter,
> because you've thrown away the baby along with the bathtub water.  The
> divergencies are precisely where the differences in generated code
> have come from, in several of the cases I've debugged over the past
> couple of months in the VTA branch.

Heh - but I claim differences in UIDs shouldn't change generated code ;)
Which unfortunately isn't true at the moment which makes your patch
necessary to preserve the -g/-g0 invariance unless we come up with
a (complete) fix for the UID problem.

I don't like it, but that doesn't change the fact (unfortunately).

Richard.

Re: Put scope blocks on a diet

[Jan Hubicka]

Hi,
> On Nov 28, 2007, Mark Mitchell <mark@codesourcery.com> wrote:
> 
> > True -- but it is consistent across runs and across hosts.  Unless the
> > UIDs vary when you add -g, which, empirically, they do.
> 
> For the record, before the July 2[46] patch, they didn't.  After that
> patch, they did.  And, ever since I started caring deeply about them
> (when I started the VTA project), that was the only regression and the
> only detail I had to fix to keep DECL_UIDs in sync comparing -g and
> -g0.
> 
> My suggestion is that, when not emitting debug information, instead of
> keeping unused decls around in the binding blocks, we replace them
> with placeholders, such that, at inlining time, instead of cloning
> the declarations they would have represented, we can merely bump up
> the decl uid counter.

I've considered of just building stripped down decls for those purposes,
however I don't like the idea much - we still will have a lot of
overhead linking those things together and producing a lot of binding
blocks that are memory consuming too.

Do you see any direct problem in the plan of simply not copying those at
-g and teaching debug info output to look into ABSTRACT_ORIGIN of the
block and see what dead variables should be added as "optimized out" I
proposed yesterday?
> 
> This *would* keep UIDs in sync with a far less destabilizing effect.
> However, I can't tell for sure that none of the removed decls would
> have made to hash tables elsewhere, which could potentially
> destabilize the compiler output by changing hashtable walk order
> because of changes in the hashtable size.  I don't quite *see* how

We do have a lot of algorithms drived by DECL_UIDs and thus those needs
to stay in sync.  Having different representation of those dead
variables not appearing in hashtables should not cause much harm IMO.
They are not apperaing in referenced/unreferenced var lists so they are
essentially invisible to all SSA code.

Honza
> unused variables would make it elsewhere, and it's simple enough that
> it's probably worth a shot.  But I can't promise it's going to work.
> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

Hi,

> On Nov 28, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:
> 
> > It depends.  We may come to the point that enabling -g doesn't consume
> > measurably more ressources.  Which would be indeed nice.
> 
> Hey, but that's easy!  We just make sure compiling without -g wastes
> lots of resources.  Then, -g won't consume more ;-)
> 
> 
> Seriously, there's no other way -g won't consume measurably more
> resources.  There's no way you can fit more information in the same
> space as less information, unless the less-information case is highly
> redundant, which indicates a poor job in packing less information.  If

We however know we can fit all the relevant information into debug
section of tramp3d object file and that is a lot smaller than the
footprints of our in-memory representation. So making our in-memory
representation more similar to dwarf would be way to get memory to very
acceptable levels.  This is motivation of the proposed plan of not
duplicating decls and instead of looking them up in dwarf2out.

> >> bootstrap-debug.  And the nice thing is that it doesn't only exercise
> >> more paths in the compiler, it is also faster than a full bootstrap
> >> with debug information enabled (the current default).  We could easily
> >> suggest it as the default testing procedure, at least on platforms on
> >> which it works, for I honestly don't know how portable bit-per-bit
> >> comparison of stripped objects is.
> 
> > Well, if we want not to break the -g vs. -g0 property then we should
> > enable this by default for regular bootstrap
> 
> +1

Alternative might be to enable this in some of our periodic testers.  If
this property does not tend to break all the time, I think it might be
enough testing without having signififcant degradation in our regular
testing cycles.  If we find this to break regularly, we can make
debugbootstrap a default. 

Debugbootstrap is good concept IMO and we should have it tested
regularly either in tester or by default.

I should also note that -fprofile-generate/-fprofile-use is another case
where we need compiler stable.  To large extend this is same problem (ie
in memory representation is slightly different in both runs, we still
need same decisions to given point of compilation).  I spent a lot of
hours tracking down divergences in compiler showing as profile mismatch,
hopefully this will also get caught more easilly with debug info testing.

Honza
> 
> > (maybe somehow distinguish debug comparison failures from code
> > comparison failures).
> 
> There's no such thing as debug comparison failures.  bootstrap-debug
> doesn't compare debug information.  It strips out debug information
> and compares only the actual code.
> 
> bootstrap-debug builds stage2 without -g (or, actually, with -g0,
> which is like no -g, if someone doesn't have that clear) and stage3
> with -g, and makes sure the generated code is the same for both.
> 
> > Just to compare -g vs. -g0 dumps?
> 
> Yes.  That makes finding out where a bootstrap-debug failure comes
> from.
> 
> > But yes, less divergence in the dumps would be nice, and you can
> > produce that with simple scripts that rip out UIDs of temporaries.
> 
> This doesn't help you locate the divergencies where they matter,
> because you've thrown away the baby along with the bathtub water.  The
> divergencies are precisely where the differences in generated code
> have come from, in several of the cases I've debugged over the past
> couple of months in the VTA branch.
> 
> So, yes, as I pointed out before, the usefulness of this property is
> quite limited, but it's also cheap to achieve with the placeholders I
> proposed.  It's even cheaper than the additional bitmaps you propose,
> my intuition tells me.
> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Mark Mitchell]

Alexandre Oliva wrote:
> On Nov 28, 2007, Mark Mitchell <mark@codesourcery.com> wrote:
> 
>> True -- but it is consistent across runs and across hosts.  Unless the
>> UIDs vary when you add -g, which, empirically, they do.
> 
> For the record, before the July 2[46] patch, they didn't. 

That's an important fact.  Given that, it's certainly not surprising
that we have algorithms that previously generated the same code with and
without -g, but now differ.

It sounds like we have three choices:

1. Revert the July patches and declare that (for now) DECL_UIDs must not
be influenced by -g.  Pros: -g doesn't affect generated code.  Cons:
compiler uses more memory than it does now, though no more than 4.2.

2. Accept that (for now) -g may affect generated code.  Pros: compiler
users less memory.  Cons: -g affects generated code.

3. Do something that makes us depend, at most, on DECL_UID order -- but
not on exact values.  Pros: -g doesn't affect generated code, compiler
uses less memory.  Cons: have to do something hard.

Is that right?  Nobody's going to like either (1) or (2) -- but as RM,
if I have to choose, I'm probably going to pick (1) because I think the
-g principle is critical.

Richard has done some version of (3) that he thinks isn't suitable for
Stage 3.  Richard, can you say what's risky about your patch?  Given
that nobody is going to like either choice (1) or choice (2), is your
patch worth considering?

Have we figured out how many places in the compiler are actually hashing
based on DECL_UID?  If it's not many, then we can wrap those hash tables
so that the order in which we walk them is stabilized.  We don't
necessarily have to do the prettiest thing right now; we just have to
find some way of containing the damage.  A basic grep is only showing
about 10 places that look like hashes of DECL_UID, so doing an audit of
these to see which of these hash tables are walked, and then interposing
a sort phase before the walk, certainly seems tractable.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 29, 2007 6:23 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Alexandre Oliva wrote:
> > On Nov 28, 2007, Mark Mitchell <mark@codesourcery.com> wrote:
> >
> >> True -- but it is consistent across runs and across hosts.  Unless the
> >> UIDs vary when you add -g, which, empirically, they do.
> >
> > For the record, before the July 2[46] patch, they didn't.
>
> That's an important fact.  Given that, it's certainly not surprising
> that we have algorithms that previously generated the same code with and
> without -g, but now differ.
>
> It sounds like we have three choices:
>
> 1. Revert the July patches and declare that (for now) DECL_UIDs must not
> be influenced by -g.  Pros: -g doesn't affect generated code.  Cons:
> compiler uses more memory than it does now, though no more than 4.2.
>
> 2. Accept that (for now) -g may affect generated code.  Pros: compiler
> users less memory.  Cons: -g affects generated code.
>
> 3. Do something that makes us depend, at most, on DECL_UID order -- but
> not on exact values.  Pros: -g doesn't affect generated code, compiler
> uses less memory.  Cons: have to do something hard.
>
> Is that right?  Nobody's going to like either (1) or (2) -- but as RM,
> if I have to choose, I'm probably going to pick (1) because I think the
> -g principle is critical.

I also would pick (1) for now, as ...

> Richard has done some version of (3) that he thinks isn't suitable for
> Stage 3.  Richard, can you say what's risky about your patch?  Given
> that nobody is going to like either choice (1) or choice (2), is your
> patch worth considering?

... the patch doesn't fix all of the problems, so we would still generate
different code with -g0 compared to -g.  Also the patch makes the
referenced_vars we iterate over dependent on garbage collection
(because referenced_vars is only a conservative estimate of the referenced
vars and may contain unreferenced ones).  This may cause new problems
which would require to re-compute referenced_vars before we rely on
them being stable.

So the patch is definitely not appropriate for stage3.

Richard.

Re: Put scope blocks on a diet

[Mark Mitchell]

Richard Guenther wrote:

>> 1. Revert the July patches and declare that (for now) DECL_UIDs must not
>> be influenced by -g.  Pros: -g doesn't affect generated code.  Cons:
>> compiler uses more memory than it does now, though no more than 4.2.
>>
>> 2. Accept that (for now) -g may affect generated code.  Pros: compiler
>> users less memory.  Cons: -g affects generated code.
>>
>> 3. Do something that makes us depend, at most, on DECL_UID order -- but
>> not on exact values.  Pros: -g doesn't affect generated code, compiler
>> uses less memory.  Cons: have to do something hard.
>>
>> Is that right?  Nobody's going to like either (1) or (2) -- but as RM,
>> if I have to choose, I'm probably going to pick (1) because I think the
>> -g principle is critical.
> 
> I also would pick (1) for now, as ...

Alexandre will presumably pick (1) as well, since, IIUC, he values
highly getting the same code without and without -g.

So, does anyone either (a) have objections to (1), or (b) want to try
(3)?  If nobody objects within 48 hours, a patch to revert the July
patches (making whatever minor changes might be necessary given the
passage of time) is preapproved.  Of course, if there are objections,
we'll continue to discuss.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Jan Hubicka]

> Alexandre Oliva wrote:
> > On Nov 28, 2007, Mark Mitchell <mark@codesourcery.com> wrote:
> > 
> >> True -- but it is consistent across runs and across hosts.  Unless the
> >> UIDs vary when you add -g, which, empirically, they do.
> > 
> > For the record, before the July 2[46] patch, they didn't. 
> 
> That's an important fact.  Given that, it's certainly not surprising
> that we have algorithms that previously generated the same code with and
> without -g, but now differ.
> 
> It sounds like we have three choices:
> 
> 1. Revert the July patches and declare that (for now) DECL_UIDs must not
> be influenced by -g.  Pros: -g doesn't affect generated code.  Cons:
> compiler uses more memory than it does now, though no more than 4.2.

It looks like I've got lost somewhere in this thread.  My understanding
is that:
  - My July 24/26 changes introducted DECL_UID divergence because of
    correlation of SCOPE block prunning and inlininer duplicating them
  - Alexandre's and mine patch comitted at Nov 26 reverted this
    behaviour so DECL_UIDs are now stable because we prune after
    inlining only
  - This patch causes memory consumption increase relative to Jul
    26-Nov26 compiler at -g, but not at -g0 that started this thread

Since there seems to be consensus that it is sane to keep DECL_UIDs
stable at least for now (and I would tend to think it is quite sane to
keep them stable forever).  Why we still need to rever Jul 24/26
patches?  This would cause even more memory consumption as the code is
still taking care to prune unneeded scope blocks at both -g and -g0
compilation.

Or is there still some divergence in DECL_UIDs in the mainline I've missed?

Jan

Re: Put scope blocks on a diet

[Mark Mitchell]

Jan Hubicka wrote:

> It looks like I've got lost somewhere in this thread. 

Me too.

> My understanding
> is that:
>   - My July 24/26 changes introducted DECL_UID divergence because of
>     correlation of SCOPE block prunning and inlininer duplicating them
>   - Alexandre's and mine patch comitted at Nov 26 reverted this
>     behaviour so DECL_UIDs are now stable because we prune after
>     inlining only
>   - This patch causes memory consumption increase relative to Jul
>     26-Nov26 compiler at -g, but not at -g0 that started this thread

So, in other words, you think we're back at the point we were before the
July changes, in terms of both -g vs. -g0 and in terms of memory
consumption?

If so, then I agree that I don't see any need for further action.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Jan Hubicka]

> Jan Hubicka wrote:
> 
> > It looks like I've got lost somewhere in this thread. 
> 
> Me too.
> 
> > My understanding
> > is that:
> >   - My July 24/26 changes introducted DECL_UID divergence because of
> >     correlation of SCOPE block prunning and inlininer duplicating them
> >   - Alexandre's and mine patch comitted at Nov 26 reverted this
> >     behaviour so DECL_UIDs are now stable because we prune after
> >     inlining only
> >   - This patch causes memory consumption increase relative to Jul
> >     26-Nov26 compiler at -g, but not at -g0 that started this thread
> 
> So, in other words, you think we're back at the point we were before the
> July changes, in terms of both -g vs. -g0 and in terms of memory
> consumption?

The patch from July 24/26 is doing two things:
  1. it looks for completely dead scopes that will never go to debug info
  and removes them early instead of keeping them in memory until
  end of compilation, when they would be just ingored by dwarf2out
  because they are dead.
  2. when debug is not generated, it removes almost all scopes to save
  memory.

Both 1. and 2. resulted in memory savings.  1. at -g and -g0, 2. at -g0
only.  I don't remember exact numbers, but 1. was already quite
considerable saving on tramp3d testcase as well as other C++ testcases.

2. is the troublesome change (I've missed the inliner/DECL_UID
correlation when doing the patch) that was targetted by Nov26 patch to enable
2. only fater inlining. 1. is safe in this respect since it does not
make any difference in between -g and -g0 compilation.

So I believe that current mainline is still keeping less debug info in
memory mainly because of 1. and in some cases because of 2. at least for
testcases with single huge function and unless I've missed some extra
problems, I think it is best to keep it as it is until we develop better
in memory representation of debug info.

Removing the July patches and November fix would just increase memory
footprint.

Honza
> 
> If so, then I agree that I don't see any need for further action.
> 
> Thanks,
> 
> -- 
> Mark Mitchell
> CodeSourcery
> mark@codesourcery.com
> (650) 331-3385 x713

Re: Put scope blocks on a diet

[Mark Mitchell]

Jan Hubicka wrote:

> Removing the July patches and November fix would just increase memory
> footprint.

No point in that, obviously.  I guess I'm going to consider this issue
closed, then, unless someone else things we still have a problem.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Put scope blocks on a diet

[Richard Guenther]

On Nov 29, 2007 7:28 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Jan Hubicka wrote:
>
> > Removing the July patches and November fix would just increase memory
> > footprint.
>
> No point in that, obviously.  I guess I'm going to consider this issue
> closed, then, unless someone else things we still have a problem.

I think we are done with the current state as of fulfilling option (1)
- no further
action needed.  I'm going to try to do option (3) for 4.4 though.

Richard.

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 29, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> Heh - but I claim differences in UIDs shouldn't change generated code ;)

Ah, yes, indeed, that makes sense.  Maybe once we fix the dependencies
on DECL_UIDs, we should have an option to introduce random noise in
UID increments, to be used for testing that this desirable property is
maintained, even if we decide to keep DECL_UIDs in sync.


It might be useful to do the same to TYPE_UIDs (I know Java will need
work to ensure same code for these), and to have garbage collection at
random points too.  Bootstrap testing with these randomization options
enabled would help expose latent bugs.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 29, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

> Alexandre will presumably pick (1) as well, since, IIUC, he values
> highly getting the same code without and without -g.

For the record, for the time being, I'd pick (4): leave as is, which
appears to be the current consensus.  But I do have some ideas that
might enable us to recover memory savings without getting DECL_UIDs
out of sync.  I don't have a timeframe for implementing them, though.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Alexandre Oliva]

On Nov 29, 2007, Jan Hubicka <jh@suse.cz> wrote:

> We however know we can fit all the relevant information into debug
> section of tramp3d object file and that is a lot smaller than the
> footprints of our in-memory representation.

True.  The dwarf representation is designed to be compact, not
efficient.  We can make that trade-off too, but I'm not sure users
will be delighted if GCC gets even slower just in order to keep debug
information represented internally in a more compact way.  In the end,
any speedups out of the smaller memory working set might be lost to
inefficiency in the internal representation.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Put scope blocks on a diet

[Jan Hubicka]

> On Nov 29, 2007, Jan Hubicka <jh@suse.cz> wrote:
> 
> > We however know we can fit all the relevant information into debug
> > section of tramp3d object file and that is a lot smaller than the
> > footprints of our in-memory representation.
> 
> True.  The dwarf representation is designed to be compact, not
> efficient.  We can make that trade-off too, but I'm not sure users
> will be delighted if GCC gets even slower just in order to keep debug
> information represented internally in a more compact way.  In the end,
> any speedups out of the smaller memory working set might be lost to
> inefficiency in the internal representation.

What makes you believe that more compact representation needs necesarily
be slower?  If we just avoid keeping around all the copies of DECLs,
then we should save both memory and compilation time, since they ought
to be rather easy to be looked up in dwarf2out via ABSTRACT_ORIGIN
pointer.

Honza
> 
> -- 
> Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
> FSF Latin America Board Member         http://www.fsfla.org/
> Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
> Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}