[PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[Nathan Froyd]

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

As noted in this message:

  http://gcc.gnu.org/ml/gcc/2007-02/msg0021.html

gcc.dg/tree-ssa/20030714-1.c started failing on sparc with -fPIC between
2006-06-18 and 2006-06-22.  The patch that caused this seems to be:

2006-06-21  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/27781
	* Makefile.in (ipa-pure-const.o): Add $(TARGET_H) dependency.
	* ipa-pure-const.c (target.h): Include.
	(analyze_function): Do not analyze functions that do not
	bind locally.

Not that the patch is wrong, mind you, but the optimizations necessary
to satisfy the dg-final directives in the testcase apparently only
activate when the function binds locally.  (The function must be marked
as pure by ipa-pure-const, which this patch inhibits.)

I certainly don't pretend to understand the IPA code or the sequence of
optimizations that's desired here, but the attached tweak to the
testcase seems to get things working with -fPIC again.  Tested on
i586-wrs-vxworks with and without -fPIC with no regressions.  OK to
commit?

-Nathan

gcc/testsuite/
2007-07-26  Nathan Froyd  <froydnj@codesourcery.com>

	* gcc.dg/tree-ssa/20030714-1.c (find_base_value): Declare as
	static so appropriate optimizations kick in.
	(find_base_value_wrapper): New function.


[-- Attachment #2: 20030714-1.patch --]
[-- Type: text/plain, Size: 644 bytes --]

Index: gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c	(revision 177473)
+++ gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c	(working copy)
@@ -15,7 +15,7 @@ struct rtx_def
   unsigned frame_related:1;
 };
 
-rtx
+static rtx
 find_base_value (src)
      rtx src;
 {
@@ -33,6 +33,12 @@ find_base_value (src)
     find_base_value (src_1);
 }
 
+rtx
+find_base_value_wrapper (src)
+     rtx src;
+{
+  return find_base_value (src);
+}
 
 /* There should be four IF conditionals.  */
 /* { dg-final { scan-tree-dump-times "if " 4 "dom3"} } */

Re: [PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[Kaveh R. GHAZI]

The testcase gcc.dg/tree-ssa/20030714-1.c is failing on the branches with
-fpic/-fPIC and this failure is a regression (apparently intentional) from
previous 4.x gcc.  More analysis below.

The test seems to rely on the code being automatically marked pure/const
by gcc during compilation.  Mainline is okay because the failing scans in
the test were removed as part of the fix for PR33826 by Kenny.  In that
PR, it was decided that recursive function must not be considered for
automatic pure/const marking.  So now the testcase on mainline passes
regardless of pic or nonpic, because the checks have been truncated to fit
into this new requirement.

On the branches, the testcase still does the original checks and gcc still
allows recursive functions to become pure/const.  And it passes in the
regular default case.  However a change was made by Richard G. to ensure
this optimization happens only if the functions bind locally.  With
-fpic/-fPIC the function doesn't bind locally and the error pops up.

Nathan proposed a fix to the testcase here in July:
http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01948.html
but I don't see that it ever got reviewed or installed.

I believe Nathan's solution is correct in that he makes the function
static so it binds locally.  By doing so, (and using a global wrapper to
get the dump) it works for -fpic/-fPIC.

Another option would be to backport Kenny's patch for PR33826 which as a
side-effect would cure this failure by removing the scans.  This might be
the correct thing to do given the analysis in the PR, however it is also
more intrusive.  I haven't bootstrapped Kenny's patch on the branches to
make sure it doesn't break anything, but I will if people think it's a
better option.

Thoughts on the best way to go?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[Richard Guenther]

On Wed, 23 Jan 2008, Kaveh R. GHAZI wrote:

> The testcase gcc.dg/tree-ssa/20030714-1.c is failing on the branches with
> -fpic/-fPIC and this failure is a regression (apparently intentional) from
> previous 4.x gcc.  More analysis below.
> 
> The test seems to rely on the code being automatically marked pure/const
> by gcc during compilation.  Mainline is okay because the failing scans in
> the test were removed as part of the fix for PR33826 by Kenny.  In that
> PR, it was decided that recursive function must not be considered for
> automatic pure/const marking.  So now the testcase on mainline passes
> regardless of pic or nonpic, because the checks have been truncated to fit
> into this new requirement.
> 
> On the branches, the testcase still does the original checks and gcc still
> allows recursive functions to become pure/const.  And it passes in the
> regular default case.  However a change was made by Richard G. to ensure
> this optimization happens only if the functions bind locally.  With
> -fpic/-fPIC the function doesn't bind locally and the error pops up.
> 
> Nathan proposed a fix to the testcase here in July:
> http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01948.html
> but I don't see that it ever got reviewed or installed.
> 
> I believe Nathan's solution is correct in that he makes the function
> static so it binds locally.  By doing so, (and using a global wrapper to
> get the dump) it works for -fpic/-fPIC.
> 
> Another option would be to backport Kenny's patch for PR33826 which as a
> side-effect would cure this failure by removing the scans.  This might be
> the correct thing to do given the analysis in the PR, however it is also
> more intrusive.  I haven't bootstrapped Kenny's patch on the branches to
> make sure it doesn't break anything, but I will if people think it's a
> better option.
> 
> Thoughts on the best way to go?

Backporting Kennys patch is the right thing to do - I don't know
why the PR was closed as fixed.

Thanks,
Richard.

Re: [PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[Kaveh R. GHAZI]

On Wed, 23 Jan 2008, Richard Guenther wrote:

> Backporting Kennys patch is the right thing to do - I don't know
> why the PR was closed as fixed.

Ok, patch below was tested against 4.2 on x86_64-unknown-linux-gnu, no
regressions.

Testing on 4.1 on i686 completed bootstrap and the testsuite is in
progress.

Okay for 4.2, (and 4.1 assuming testsuite finished okay)?

		Thanks,
		--Kaveh


2008-01-24  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	Backport:
	2008-01-10  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* gcc.dg/pr33826.c: Require nonpic.

	2007-11-07  Kenneth Zadeck <zadeck@naturalbridge.com>

	PR middle-end/33826
	* ipa-pure-const (static_execute): Added code to keep recursive
	functions from being marked as pure or const.
	* ipa-utils (searchc): Fixed comment.

	2007-11-08  Kenneth Zadeck <zadeck@naturalbridge.com>

	PR middle-end/33826
	* gcc.dg/pr33826.c: New.
	* gcc.dg/tree-ssa/20030714-1.c: Removed two tests that depend on
	recursive functions being marked pure or const.

diff -rup orig/egcc-4.2-SVN20080122/gcc/ipa-pure-const.c egcc-4.2-SVN20080122/gcc/ipa-pure-const.c
--- orig/egcc-4.2-SVN20080122/gcc/ipa-pure-const.c	2008-01-03 23:38:08.000000000 +0100
+++ egcc-4.2-SVN20080122/gcc/ipa-pure-const.c	2008-01-24 02:50:38.000000000 +0100
@@ -638,6 +638,7 @@ static_execute (void)
   for (i = 0; i < order_pos; i++ )
     {
       enum pure_const_state_e pure_const_state = IPA_CONST;
+      int count = 0;
       node = order[i];

       /* Find the worst state for any node in the cycle.  */
@@ -654,11 +655,40 @@ static_execute (void)
 	  if (!w_l->state_set_in_source)
 	    {
 	      struct cgraph_edge *e;
+	      count++;
+
+	      /* FIXME!!!  Because of pr33826, we cannot have either
+		 immediate or transitive recursive functions marked as
+		 pure or const because dce can delete a function that
+		 is in reality an infinite loop.  A better solution
+		 than just outlawing them is to add another bit the
+		 functions to distinguish recursive from non recursive
+		 pure and const function.  This would allow the
+		 recursive ones to be cse'd but not dce'd.  In this
+		 same vein, we could allow functions with loops to
+		 also be cse'd but not dce'd.
+
+		 Unfortunately we are late in stage 3, and the fix
+		 described above is is not appropriate.  */
+	      if (count > 1)
+		{
+		  pure_const_state = IPA_NEITHER;
+		  break;
+		}
+
 	      for (e = w->callees; e; e = e->next_callee)
 		{
 		  struct cgraph_node *y = e->callee;
 		  /* Only look at the master nodes and skip external nodes.  */
 		  y = cgraph_master_clone (y);
+
+		  /* Check for immediate recursive functions.  See the
+		     FIXME above.  */
+		  if (w == y)
+		    {
+		      pure_const_state = IPA_NEITHER;
+		      break;
+		    }
 		  if (y)
 		    {
 		      funct_state y_l = get_function_state (y);
diff -rup orig/egcc-4.2-SVN20080122/gcc/ipa-utils.c egcc-4.2-SVN20080122/gcc/ipa-utils.c
--- orig/egcc-4.2-SVN20080122/gcc/ipa-utils.c	2008-01-03 23:38:08.000000000 +0100
+++ egcc-4.2-SVN20080122/gcc/ipa-utils.c	2008-01-24 02:50:38.000000000 +0100
@@ -76,7 +76,7 @@ struct searchc_env {
    has been customized for cgraph_nodes.  The env parameter is because
    it is recursive and there are no nested functions here.  This
    function should only be called from itself or
-   cgraph_reduced_inorder.  ENV is a stack env and would be
+   ipa_utils_reduced_inorder.  ENV is a stack env and would be
    unnecessary if C had nested functions.  V is the node to start
    searching from.  */

diff -rup orig/egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/pr33826.c egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/pr33826.c
--- orig/egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/pr33826.c	2008-01-24 02:53:00.000000000 +0100
+++ egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/pr33826.c	2008-01-24 02:54:57.000000000 +0100
@@ -0,0 +1,41 @@
+/* Regression test for PR middle-end/33826 */
+/* Verify that recursive functions cannot be pure or const.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target nonpic } */
+/* { dg-options "-O1 -fdump-ipa-pure-const" } */
+
+int recurese1 (int i)
+{
+  return recurse1 (i+1);
+}
+
+int recurse2a (int i)
+{
+  return recurse2b (i+1);
+}
+
+int recurse2b (int i)
+{
+  return recurse2a (i+1);
+}
+
+int norecurse1a (int i)
+{
+  return norecurse1b (i+1);
+}
+
+int norecurse1b (int i)
+{
+  return i+1;
+}
+
+/* { dg-final { scan-ipa-dump "found to be const: norecurse1a" "pure-const" } } */
+/* { dg-final { scan-ipa-dump "found to be const: norecurse1b" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be pure: recurse1" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be pure: recurse2a" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be pure: recurse2b" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be const: recurse1" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be const: recurse2a" "pure-const" } } */
+/* { dg-final { scan-ipa-dump-not "found to be const: recurse2b" "pure-const" } } */
+/* { dg-final { cleanup-ipa-dump "pure-const" } } */
diff -rup orig/egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c
--- orig/egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c	2008-01-03 23:34:53.000000000 +0100
+++ egcc-4.2-SVN20080122/gcc/testsuite/gcc.dg/tree-ssa/20030714-1.c	2008-01-24 02:50:38.000000000 +0100
@@ -34,13 +34,6 @@ find_base_value (src)
 }


-/* There should be four IF conditionals.  */
-/* { dg-final { scan-tree-dump-times "if " 4 "dom3"} } */
-
 /* There should be no casts to short unsigned int.  */
 /* { dg-final { scan-tree-dump-times "\\(short unsigned int\\)" 0 "dom3"} } */

-/* There should be two loads of ->code.  */
-/* { dg-final { scan-tree-dump-times "->code" 2 "dom3"} } */
-
-/* { dg-final { cleanup-tree-dump "dom3" } } */

Re: [PATCH,testsuite] fix gcc.dg/tree-ssa/20030714-1.c with -fPIC

[Richard Guenther]

On Thu, 24 Jan 2008, Kaveh R. GHAZI wrote:

> On Wed, 23 Jan 2008, Richard Guenther wrote:
> 
> > Backporting Kennys patch is the right thing to do - I don't know
> > why the PR was closed as fixed.
> 
> Ok, patch below was tested against 4.2 on x86_64-unknown-linux-gnu, no
> regressions.
> 
> Testing on 4.1 on i686 completed bootstrap and the testsuite is in
> progress.
> 
> Okay for 4.2, (and 4.1 assuming testsuite finished okay)?

Ok.

Thanks,
Richard.