Re: [PATCH] Fix common_pointer_type for fn pointers where one has __attribute__((const)) and one does not (PR tree-optimization/32139)

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, May 31, 2007 at 08:57:12PM +0000, Joseph S. Myers wrote:
> On Thu, 31 May 2007, Jakub Jelinek wrote:
> 
> > On FUNCTION_TYPE TREE_READONLY means the fn has __attribute__((const))
> > and that should be merged IMHO conservatively, TREE_READONLY on the
> > common fntype onlyl if both functions are __attribute__((const)),
> > otherwise we can optimize out some invocations even when they have
> > side-effects.
> 
> I think the same should apply to the volatile qualifier (used for 
> noreturn) as well; build_c_cast treats them the same for -Wcast-qual 
> warnings.

Good idea, below is a new version of the patch.

I wonder if creating a union of the attributes in common_poiner_type
is a good idea.  I would guess that for many function attributes
we want intersection or even more complicated processing though
(say, if one FUNCTION_TYPE has nonnull attribute and the other not,
or one has nonnull(1,2) and the other nonnull(2,3), the result should
be nonnull(2), etc.).
Furthermore, I'm not sure if it is a good idea to represent
__attribute__((const)) as TYPE_READONLY on the FUNCTION_TYPE.

Say this testcase
int f1 (void);
int f2 (void) __attribute__ ((const));
int f3 (void);
int f4 (void) __attribute__ ((noreturn));
int f5 (void);
int f6 (void) __attribute__ ((pure));

int
test1 (int x)
{
  __typeof (x == 10000 ? &f1 : &f2) fptr1;
  fptr1 = (x == 10000 ? &f1 : &f2);
  int a = fptr1 ();
  int b = fptr1 ();
  return a + b;
}

int
test2 (int x)
{
  __typeof (x == 10000 ? &f3 : &f4) fptr2;
  fptr2 = (x == 10000 ? &f3 : &f4);
  int a = fptr2 ();
  int b = fptr2 ();
  return a + b;
}

int
test3 (int x)
{
  __typeof (x == 10000 ? &f5 : &f6) fptr3;
  fptr3 = (x == 10000 ? &f5 : &f6);
  int a = fptr3 ();
  int b = fptr3 ();
  return a + b;
}
doesn't compile even with my patch below (which fixes test2 case,
without it only one fptr2 call is done even for test2 (10000) and
that's the last insn in the routine).  The reason is that
build_unary_op (ADDR_EXPR, <function_decl f2>, 0)
will make the ADDR_EXPR TREE_READONLY iff <function_decl f2>
is TREE_READONLY.  If that's the only place, it surely can be
special cased, but I fear there are many places all around.
test3 works, as GCC ATM doesn't represent pure attribute on types,
can't we just stick "pure" into TYPE_ATTRIBUTES and handle it?
If we did that, we'd of course need to handle that in common_pointer_type
too (e.g. merging const and pure functions result in pure, etc.).

2007-06-01  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/32139
	* c-typeck.c (common_pointer_type): Set TYPE_READONLY
	and TYPE_VOLATILE on the merged pointed to FUNCTION_TYPE
	only if both pointed_to_1 and pointed_to_2 are TYPE_READONLY
	resp. TYPE_VOLATILE.

	* gcc.c-torture/compile/20070531-1.c: New test.

--- gcc/c-typeck.c.jj	2007-04-25 10:13:52.000000000 +0200
+++ gcc/c-typeck.c	2007-06-01 10:51:53.000000000 +0200
@@ -499,6 +499,7 @@ common_pointer_type (tree t1, tree t2)
   tree pointed_to_1, mv1;
   tree pointed_to_2, mv2;
   tree target;
+  int type_quals;
 
   /* Save time if the two types are the same.  */
 
@@ -526,10 +527,19 @@ common_pointer_type (tree t1, tree t2)
   if (TREE_CODE (mv2) != ARRAY_TYPE)
     mv2 = TYPE_MAIN_VARIANT (pointed_to_2);
   target = composite_type (mv1, mv2);
-  t1 = build_pointer_type (c_build_qualified_type
-			   (target,
-			    TYPE_QUALS (pointed_to_1) |
-			    TYPE_QUALS (pointed_to_2)));
+  type_quals = TYPE_QUALS (pointed_to_1) | TYPE_QUALS (pointed_to_2);
+  if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
+    {
+      /* TYPE_READONLY and TYPE_VOLATILE on FUNCTION_TYPE should be
+	 logically ANDed, not ORed, as if one function is
+	 __attribute__((const)) and the other is not, the common type
+	 must be conservatively not __attribute__((const))
+	 and similarly for __attribute__((noreturn)).  */
+      type_quals &= ~(TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE);
+      type_quals |= (TYPE_QUALS (pointed_to_1) & TYPE_QUALS (pointed_to_2))
+		    & (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE);
+    }
+  t1 = build_pointer_type (c_build_qualified_type (target, type_quals));
   return build_type_attribute_variant (t1, attributes);
 }
 
--- gcc/testsuite/gcc.c-torture/compile/20070531-1.c.jj	2007-05-31 13:47:22.000000000 +0200
+++ gcc/testsuite/gcc.c-torture/compile/20070531-1.c	2007-06-01 10:57:15.000000000 +0200
@@ -0,0 +1,11 @@
+/* PR tree-optimization/32139 */
+int foo (void);
+int bar (void) __attribute__ ((const));
+
+int
+test (int x)
+{
+  int a = (x == 10000 ? foo : bar) ();
+  int b = (x == 10000 ? foo : bar) ();
+  return a + b;
+}


	Jakub