diff mbox

[trunk<-vta] Re: [vtab] Permit coalescing of user variables

Message ID orfwnpg46x.fsf@livre.localdomain
State New
Headers show

Commit Message

Alexandre Oliva June 4, 2011, 12:40 p.m. UTC 
On Oct 13, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jun  1, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:
>> A long time ago, when variable tracking at assignments was just a
>> distant dream, we ran into one of the first contentious points, which
>> had to do with coalescing SSA names on copyrename.

>> On the one hand, coalescing unrelated SSA names made for better code (at
>> least in theory) but poorer debug information; on the other hand,
>> refraining from coalescing them exploded compile-time memory use.

>> We currently implement a trade-off by which variables inlined from other
>> functions can be coalesced, so as to save compile-time memory, reduce
>> abstraction penalties and retain debug information for out-of-line
>> functions.

>> The patch below (ping) implements two other possibilities: refraining
>> from coalescing even inlined SSA names, which might enable better debug
>> information to be generated, and enabling coalescing of all related
>> variables, for better code at the expense of debug information.

>> VTA doesn't really care which of the 3 possibilities is used, it works
>> equally well with all of them.

> On Jun  1, 2009, Alexandre Oliva <aoliva@redhat.com> also wrote:

>> And the patch below changes the default so that we can optimize more.

> This patch combines the two patches described above, now that VTA is
> enabled by default.

This is an updated version of the patch, adjusting the testcases that
didn't expect this kind of variable coalescing.  Regstrapped on
x86_64-linux-gnu and i686-linux-gnu, along with a patch for a latent
cprop problem that caused a compare-debug failure in the ada rts.
Ok to install?

Comments

Jakub Jelinek June 4, 2011, 1:02 p.m. UTC | #1 
On Sat, Jun 04, 2011 at 09:40:38AM -0300, Alexandre Oliva wrote:

The following changes all look wrong to me, they make the tests totally
useless.  If both f and g are used in real code after the asm volatile, then
the both f and g will likely live in some register or memory.
The whole point of the construct in the tests is that f has at that spot
a reg or mem location, but g isn't present anywhere anymore (as the compiler
doesn't or shouldn't know that asm volatile hasn't changed f), thus it
should represent them as bswap/clz/ctz/rotate.

> --- gcc/testsuite/gcc.dg/guality/bswaptest.c.orig	2011-06-04 09:35:38.954014890 -0300
> +++ gcc/testsuite/gcc.dg/guality/bswaptest.c	2011-06-04 09:35:51.345054255 -0300
> @@ -10,7 +10,7 @@ foo (long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) int
> @@ -20,7 +20,7 @@ bar (int x)
>    int g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  int
> Index: gcc/testsuite/gcc.dg/guality/clztest.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/guality/clztest.c.orig	2011-06-04 09:35:39.202015678 -0300
> +++ gcc/testsuite/gcc.dg/guality/clztest.c	2011-06-04 09:36:17.710136856 -0300
> @@ -10,7 +10,7 @@ foo (long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noinline, noclone)) long
> @@ -20,7 +20,7 @@ bar (long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  int
> Index: gcc/testsuite/gcc.dg/guality/ctztest.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/guality/ctztest.c.orig	2011-06-04 09:35:39.463016509 -0300
> +++ gcc/testsuite/gcc.dg/guality/ctztest.c	2011-06-04 09:36:33.143184587 -0300
> @@ -10,7 +10,7 @@ foo (long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noinline, noclone)) long
> @@ -20,7 +20,7 @@ bar (long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  int
> Index: gcc/testsuite/gcc.dg/guality/rotatetest.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/guality/rotatetest.c.orig	2011-06-04 09:32:07.155300180 -0300
> +++ gcc/testsuite/gcc.dg/guality/rotatetest.c	2011-06-04 09:34:46.757846376 -0300
> @@ -10,7 +10,7 @@ f1 (unsigned long x)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) long
> @@ -20,7 +20,7 @@ f2 (unsigned long x, int y)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) long
> @@ -30,7 +30,7 @@ f3 (unsigned long x, int y)
>    long g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 32 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) unsigned int
> @@ -40,7 +40,7 @@ f4 (unsigned int x)
>    unsigned int g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 42 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) unsigned int
> @@ -50,7 +50,7 @@ f5 (unsigned int x, int y)
>    unsigned int g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 52 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  __attribute__((noclone, noinline)) unsigned int
> @@ -60,7 +60,7 @@ f6 (unsigned int x, int y)
>    unsigned int g = f;
>    asm volatile ("" : "+r" (f));
>    vv++;		/* { dg-final { gdb-test 62 "g" "f" } } */
> -  return f;
> +  return f - g;
>  }
>  
>  int

	Jakub
Alexandre Oliva June 5, 2011, 9:01 p.m. UTC | #2 
On Jun  4, 2011, Jakub Jelinek <jakub@redhat.com> wrote:

> The following changes all look wrong to me, they make the tests totally
> useless.  If both f and g are used in real code after the asm volatile, then
> the both f and g will likely live in some register or memory.
> The whole point of the construct in the tests is that f has at that spot
> a reg or mem location, but g isn't present anywhere anymore (as the compiler
> doesn't or shouldn't know that asm volatile hasn't changed f), thus it
> should represent them as bswap/clz/ctz/rotate.

I see.  I'll try to figure out why that didn't work.  Maybe the input
operands were clobbered or something, or maybe g used to be saved
elsewhere before and now it no longer is.
Alexandre Oliva June 6, 2011, 2:37 a.m. UTC | #3 
On Jun  5, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jun  4, 2011, Jakub Jelinek <jakub@redhat.com> wrote:
>> The following changes all look wrong to me, they make the tests totally
>> useless.  If both f and g are used in real code after the asm volatile, then
>> the both f and g will likely live in some register or memory.
>> The whole point of the construct in the tests is that f has at that spot
>> a reg or mem location, but g isn't present anywhere anymore (as the compiler
>> doesn't or shouldn't know that asm volatile hasn't changed f), thus it
>> should represent them as bswap/clz/ctz/rotate.

> I see.  I'll try to figure out why that didn't work.

So, I tried to duplicate the problem and failed.  The cut&pasto fix
r174632 fixed the only two cases that had failed, on x86_64-linux-gnu
and only at -O1.  I suppose the wrong bit width made the expression
unsuitable for a rotate or debug expr, but the fix made the tests pass
again, so I withdraw the “fixes” for those testcases: they work even
with coalescing fully enabled.  Thanks for catching this.
diff mbox

Patch

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

	* common.opt (ftree-coalesce-inlined-vars): New.
	(ftree-coalesce-vars): New.
	* doc/invoke.texi: Document them.
	* tree-ssa-copyrename.c (copy_rename_partition_coalesce):
	Implement them.

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

	* g++.dg/tree-ssa/ivopts-2.C: Adjust for coalescing.
	* gcc.dg/tree-ssa/forwprop-11.c: Likewise.
	* gcc.dg/tree-ssa/ssa-fre-1.c: Likewise.
	* gcc.dg/guality/bswaptest.c: Avoid coalescing and early death
	of test variable.
	* gcc.dg/guality/clztest.c: Likewise.
	* gcc.dg/guality/ctztest.c: Likewise.
	* gcc.dg/guality/rotatetest.c: Likewise.

Index: gcc/common.opt
===================================================================
--- gcc/common.opt.orig	2011-06-04 09:00:30.005102549 -0300
+++ gcc/common.opt	2011-06-04 09:01:29.199347556 -0300
@@ -1898,6 +1898,14 @@  ftree-ch
 Common Report Var(flag_tree_ch) Optimization
 Enable loop header copying on trees
 
+ftree-coalesce-inlined-vars
+Common Report Var(flag_ssa_coalesce_vars,1) Init(2) RejectNegative Optimization
+Enable coalescing of copy-related user variables that are inlined
+
+ftree-coalesce-vars
+Common Report Var(flag_ssa_coalesce_vars,2) Optimization
+Enable coalescing of all copy-related user variables
+
 ftree-copyrename
 Common Report Var(flag_tree_copyrename) Optimization
 Replace SSA temporaries with better names in copies
Index: gcc/tree-ssa-copyrename.c
===================================================================
--- gcc/tree-ssa-copyrename.c.orig	2011-06-04 09:00:30.160103200 -0300
+++ gcc/tree-ssa-copyrename.c	2011-06-04 09:01:29.252347770 -0300
@@ -194,20 +194,21 @@  copy_rename_partition_coalesce (var_map 
   ign1 = TREE_CODE (root1) == VAR_DECL && DECL_IGNORED_P (root1);
   ign2 = TREE_CODE (root2) == VAR_DECL && DECL_IGNORED_P (root2);
 
-  /* Never attempt to coalesce 2 user variables unless one is an inline
-     variable.  */
+  /* Refrain from coalescing user variables, if requested.  */
   if (!ign1 && !ign2)
     {
-      if (DECL_FROM_INLINE (root2))
+      if (flag_ssa_coalesce_vars && DECL_FROM_INLINE (root2))
         ign2 = true;
-      else if (DECL_FROM_INLINE (root1))
+      else if (flag_ssa_coalesce_vars && DECL_FROM_INLINE (root1))
 	ign1 = true;
-      else
+      else if (flag_ssa_coalesce_vars != 2)
 	{
 	  if (debug)
 	    fprintf (debug, " : 2 different USER vars. No coalesce.\n");
 	  return false;
 	}
+      else
+	ign2 = true;
     }
 
   /* If both values have default defs, we can't coalesce.  If only one has a
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi.orig	2011-06-04 09:00:30.665105322 -0300
+++ gcc/doc/invoke.texi	2011-06-04 09:01:29.479348698 -0300
@@ -395,8 +395,8 @@  Objective-C and Objective-C++ Dialects}.
 -fsignaling-nans -fsingle-precision-constant -fsplit-ivs-in-unroller @gol
 -fsplit-wide-types -fstack-protector -fstack-protector-all @gol
 -fstrict-aliasing -fstrict-overflow -fthread-jumps -ftracer @gol
--ftree-bit-ccp @gol
--ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol
+-ftree-bit-ccp -ftree-builtin-call-dce -ftree-ccp -ftree-ch @gol
+-ftree-coalesce-inline-vars -ftree-coalesce-vars -ftree-copy-prop @gol
 -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
 -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol
 -ftree-loop-if-convert-stores -ftree-loop-im @gol
@@ -7231,6 +7231,22 @@  temporaries to other variables at copy l
 variable names which more closely resemble the original variables.  This flag
 is enabled by default at @option{-O} and higher.
 
+@item -ftree-coalesce-inlined-vars
+A more limited form of @option{-ftree-coalesce-vars}, that performs
+the transformation if at least one of the variables was inlined.  This
+may harm debug information of such inlined variables, but it will keep
+variables of the main function apart from each other, such that they
+are more likely to contain the expected values in a debugging session.
+This was the default in GCC versions older than 4.7.
+
+@item -ftree-coalesce-vars
+Tell the compiler to attempt to combine small user variables that
+don't need to live in memory, when it finds a copy between them.  This
+may severely limit the ability to debug an optimized program compiled
+without @option{-fvar-tracking-assignments}.  In the negated form,
+this flag prevents SSA coalescing of user variables, including inlined
+ones.  This option is enabled by default.
+
 @item -ftree-ter
 @opindex ftree-ter
 Perform temporary expression replacement during the SSA->normal phase.  Single
Index: gcc/testsuite/g++.dg/tree-ssa/ivopts-2.C
===================================================================
--- gcc/testsuite/g++.dg/tree-ssa/ivopts-2.C.orig	2011-06-04 09:13:29.613784393 -0300
+++ gcc/testsuite/g++.dg/tree-ssa/ivopts-2.C	2011-06-04 09:16:24.688235836 -0300
@@ -7,5 +7,5 @@  void test (int *b, int *e, int stride)
       *p = 1;
   }
 
-/* { dg-final { scan-tree-dump-times "PHI <p" 1 "ivopts"} } */
+/* { dg-final { scan-tree-dump-times "PHI <\[pb\]" 1 "ivopts"} } */
 /* { dg-final { cleanup-tree-dump "ivopts" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/forwprop-11.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/forwprop-11.c.orig	2011-06-04 09:08:19.363853778 -0300
+++ gcc/testsuite/gcc.dg/tree-ssa/forwprop-11.c	2011-06-04 09:09:12.127024253 -0300
@@ -16,5 +16,5 @@  int g(int *p, int n)
   return q[-1];
 }
 
-/* { dg-final { scan-tree-dump-times "= MEM\\\[\\\(int \\\*\\\)a_.. \\\+ 4B\\\];" 2 "forwprop1" } } */
+/* { dg-final { scan-tree-dump-times "= MEM\\\[\\\(int \\\*\\\)\[ap\]_.. \\\+ 4B\\\];" 2 "forwprop1" } } */
 /* { dg-final { cleanup-tree-dump "forwprop1" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-1.c.orig	2011-06-04 09:11:37.164466849 -0300
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-1.c	2011-06-04 09:11:41.365479111 -0300
@@ -11,5 +11,5 @@  int f(int *a)
   return *c + t;
 }
 
-/* { dg-final { scan-tree-dump "Replaced \\\*c_\[^\n\].*with t_" "fre1" } } */
+/* { dg-final { scan-tree-dump "Replaced \\\*\[ac\]_\[^\n\].*with t_" "fre1" } } */
 /* { dg-final { cleanup-tree-dump "fre1" } } */
Index: gcc/testsuite/gcc.dg/guality/bswaptest.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/bswaptest.c.orig	2011-06-04 09:35:38.954014890 -0300
+++ gcc/testsuite/gcc.dg/guality/bswaptest.c	2011-06-04 09:35:51.345054255 -0300
@@ -10,7 +10,7 @@  foo (long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) int
@@ -20,7 +20,7 @@  bar (int x)
   int g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 int
Index: gcc/testsuite/gcc.dg/guality/clztest.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/clztest.c.orig	2011-06-04 09:35:39.202015678 -0300
+++ gcc/testsuite/gcc.dg/guality/clztest.c	2011-06-04 09:36:17.710136856 -0300
@@ -10,7 +10,7 @@  foo (long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noinline, noclone)) long
@@ -20,7 +20,7 @@  bar (long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 int
Index: gcc/testsuite/gcc.dg/guality/ctztest.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/ctztest.c.orig	2011-06-04 09:35:39.463016509 -0300
+++ gcc/testsuite/gcc.dg/guality/ctztest.c	2011-06-04 09:36:33.143184587 -0300
@@ -10,7 +10,7 @@  foo (long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noinline, noclone)) long
@@ -20,7 +20,7 @@  bar (long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 int
Index: gcc/testsuite/gcc.dg/guality/rotatetest.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/rotatetest.c.orig	2011-06-04 09:32:07.155300180 -0300
+++ gcc/testsuite/gcc.dg/guality/rotatetest.c	2011-06-04 09:34:46.757846376 -0300
@@ -10,7 +10,7 @@  f1 (unsigned long x)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 12 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) long
@@ -20,7 +20,7 @@  f2 (unsigned long x, int y)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 22 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) long
@@ -30,7 +30,7 @@  f3 (unsigned long x, int y)
   long g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 32 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) unsigned int
@@ -40,7 +40,7 @@  f4 (unsigned int x)
   unsigned int g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 42 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) unsigned int
@@ -50,7 +50,7 @@  f5 (unsigned int x, int y)
   unsigned int g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 52 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 __attribute__((noclone, noinline)) unsigned int
@@ -60,7 +60,7 @@  f6 (unsigned int x, int y)
   unsigned int g = f;
   asm volatile ("" : "+r" (f));
   vv++;		/* { dg-final { gdb-test 62 "g" "f" } } */
-  return f;
+  return f - g;
 }
 
 int