[PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[Jakub Jelinek]

Hi!

In the following testcase sqrt is declared as unprototyped function
and so it depends on what type has the argument passed to it.
If an argument of incorrect type is passed, the sqrt comparison
simplification can create invalid GIMPLE.

The patch fixes it by punting if there is a mismatch of types.
As I didn't want to reindent 133 lines, the first hunk contains an
ugly hack with if (false).  If you prefer reindentation, I can do that
too.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-04-05  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/105150
	* match.pd (sqrt (x) cmp real_cst, sqrt (x) cmp sqrt (y)): Punt
	if sqrt operand has incompatible types.

	* gcc.dg/pr105150.c: New test.

--- gcc/match.pd.jj	2022-03-18 18:32:36.000000000 +0100
+++ gcc/match.pd	2022-04-04 19:49:28.621934784 +0200
@@ -4927,6 +4927,10 @@ (define_operator_list SYNC_FETCH_AND_AND
    (simplify
     (cmp (sq @0) REAL_CST@1)
     (switch
+     (if (!types_match (TREE_TYPE (@0), TREE_TYPE (@1)))
+      /* Punt if there is a type mismatch.  */
+      (if (false)
+       @1))
      (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@1)))
       (switch
        /* sqrt(x) < y is always false, if y is negative.  */
@@ -5062,7 +5066,7 @@ (define_operator_list SYNC_FETCH_AND_AND
    /* Transform sqrt(x) cmp sqrt(y) -> x cmp y.  */
    (simplify
     (cmp (sq @0) (sq @1))
-      (if (! HONOR_NANS (@0))
+      (if (! HONOR_NANS (@0) && types_match (TREE_TYPE (@0), TREE_TYPE (@1)))
 	(cmp @0 @1))))))
 
 /* Optimize various special cases of (FTYPE) N CMP (FTYPE) M.  */
--- gcc/testsuite/gcc.dg/pr105150.c.jj	2022-04-04 19:53:03.597935060 +0200
+++ gcc/testsuite/gcc.dg/pr105150.c	2022-04-04 19:54:55.734370333 +0200
@@ -0,0 +1,8 @@
+/* PR tree-optimization/105150 */
+/* { dg-options "-w -Ofast" } */
+
+#define A(name) __typeof (__builtin_##name (0)) name (); \
+  float name##1 () { return !name (1); } \
+  double name##2 () { return name (1.0L); }
+#define B(name) A(name) A(name##l)
+B (sqrt)

	Jakub

Re: [PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[Richard Biener]

On Tue, 5 Apr 2022, Jakub Jelinek wrote:

> Hi!
> 
> In the following testcase sqrt is declared as unprototyped function
> and so it depends on what type has the argument passed to it.
> If an argument of incorrect type is passed, the sqrt comparison
> simplification can create invalid GIMPLE.
> 
> The patch fixes it by punting if there is a mismatch of types.
> As I didn't want to reindent 133 lines, the first hunk contains an
> ugly hack with if (false).  If you prefer reindentation, I can do that
> too.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

So why does this get pas gimple_call_combined_fn ()?  Are those
internal functions at the point we match them?  Otherwise we
invoke gimple_builtin_call_types_compatible_p on them.  If
they are internal functions why did we rewrite the calls to them
when the types do not match?  If gimple_builtin_call_types_compatible_p
is fine then why did the frontend assign BUILT_IN_SQRT to the
function decls?

Richard.

> 2022-04-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/105150
> 	* match.pd (sqrt (x) cmp real_cst, sqrt (x) cmp sqrt (y)): Punt
> 	if sqrt operand has incompatible types.
> 
> 	* gcc.dg/pr105150.c: New test.
> 
> --- gcc/match.pd.jj	2022-03-18 18:32:36.000000000 +0100
> +++ gcc/match.pd	2022-04-04 19:49:28.621934784 +0200
> @@ -4927,6 +4927,10 @@ (define_operator_list SYNC_FETCH_AND_AND
>     (simplify
>      (cmp (sq @0) REAL_CST@1)
>      (switch
> +     (if (!types_match (TREE_TYPE (@0), TREE_TYPE (@1)))
> +      /* Punt if there is a type mismatch.  */
> +      (if (false)
> +       @1))
>       (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@1)))
>        (switch
>         /* sqrt(x) < y is always false, if y is negative.  */
> @@ -5062,7 +5066,7 @@ (define_operator_list SYNC_FETCH_AND_AND
>     /* Transform sqrt(x) cmp sqrt(y) -> x cmp y.  */
>     (simplify
>      (cmp (sq @0) (sq @1))
> -      (if (! HONOR_NANS (@0))
> +      (if (! HONOR_NANS (@0) && types_match (TREE_TYPE (@0), TREE_TYPE (@1)))
>  	(cmp @0 @1))))))
>  
>  /* Optimize various special cases of (FTYPE) N CMP (FTYPE) M.  */
> --- gcc/testsuite/gcc.dg/pr105150.c.jj	2022-04-04 19:53:03.597935060 +0200
> +++ gcc/testsuite/gcc.dg/pr105150.c	2022-04-04 19:54:55.734370333 +0200
> @@ -0,0 +1,8 @@
> +/* PR tree-optimization/105150 */
> +/* { dg-options "-w -Ofast" } */
> +
> +#define A(name) __typeof (__builtin_##name (0)) name (); \
> +  float name##1 () { return !name (1); } \
> +  double name##2 () { return name (1.0L); }
> +#define B(name) A(name) A(name##l)
> +B (sqrt)
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)

Re: [PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[Jakub Jelinek]

On Tue, Apr 05, 2022 at 10:50:01AM +0200, Richard Biener wrote:
> > In the following testcase sqrt is declared as unprototyped function
> > and so it depends on what type has the argument passed to it.
> > If an argument of incorrect type is passed, the sqrt comparison
> > simplification can create invalid GIMPLE.
> > 
> > The patch fixes it by punting if there is a mismatch of types.
> > As I didn't want to reindent 133 lines, the first hunk contains an
> > ugly hack with if (false).  If you prefer reindentation, I can do that
> > too.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> So why does this get pas gimple_call_combined_fn ()?  Are those
> internal functions at the point we match them?  Otherwise we
> invoke gimple_builtin_call_types_compatible_p on them.  If
> they are internal functions why did we rewrite the calls to them
> when the types do not match?  If gimple_builtin_call_types_compatible_p
> is fine then why did the frontend assign BUILT_IN_SQRT to the
> function decls?

For the unprototyped functions like double sqrt (); they can be called in a
valid way or they might not, so I think having DECL_BUILT_IN_CLASS other
than BUILT_IN_NORMAL is right.

The problem is that we don't do proper checking.

In generic-match.cc, we just call get_call_combined_fn which performs no
checking of the arguments whatsoever.

In gimple-match.cc, we call gimple_call_combined_fn, which performs bad
checking of the arguments.

When not an internal function (which doesn't need argument checking, the
compiler guarantees it is right), the former does just:
  tree fndecl = get_callee_fndecl (call);
  if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
In GIMPLE, we call:
  && gimple_builtin_call_types_compatible_p (stmt, fndecl)
but that is insufficient, that verifies whether the arguments passed to
GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
be the user declaration, so doesn't have to match exactly the builtin
as predeclared by builtins.def etc. (though, there is the cotcha that say
for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
we use additional:
  tree callee = gimple_call_fndecl (stmt);
  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
  if (decl
      && decl != callee
      && !gimple_builtin_call_types_compatible_p (stmt, decl))
    return false;

For this particular testcase, seems the problem is just the generic folding,
if I add && GIMPLE to the sqrt comparison foldings, it doesn't ICE anymore,
so just adding generic_builtin_call_types_compatible_p (basically copy
gimple_builtin_call_types_compatible_p) might be enough.
Though a big question is what it should do, using useless_type_conversion_p
might be needed so that it can deal e.g. with the FILE * vs. void *
differences (and various others), on the other side, what e.g. the exact
sqrt comparison folding requires to be much more strict, as it turns
(cmp (sq @0) REAL_CST@1)
to
(cmp @0 @1)
and similarly for
(cmp (sq @0) (sq @1))
and that requires exact type match, no?

So one possibility would be to use useless_type_conversion_p but actually
not write (cmp @0 @1) in such cases, but (cmp (convert:op1type @0) @1)
or so?

	Jakub

Re: [PATCH] match.pd: Punt on optimizing sqrt with incorrect arg type [PR105150]

[Richard Biener]

On Tue, 5 Apr 2022, Jakub Jelinek wrote:

> On Tue, Apr 05, 2022 at 10:50:01AM +0200, Richard Biener wrote:
> > > In the following testcase sqrt is declared as unprototyped function
> > > and so it depends on what type has the argument passed to it.
> > > If an argument of incorrect type is passed, the sqrt comparison
> > > simplification can create invalid GIMPLE.
> > > 
> > > The patch fixes it by punting if there is a mismatch of types.
> > > As I didn't want to reindent 133 lines, the first hunk contains an
> > > ugly hack with if (false).  If you prefer reindentation, I can do that
> > > too.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > So why does this get pas gimple_call_combined_fn ()?  Are those
> > internal functions at the point we match them?  Otherwise we
> > invoke gimple_builtin_call_types_compatible_p on them.  If
> > they are internal functions why did we rewrite the calls to them
> > when the types do not match?  If gimple_builtin_call_types_compatible_p
> > is fine then why did the frontend assign BUILT_IN_SQRT to the
> > function decls?
> 
> For the unprototyped functions like double sqrt (); they can be called in a
> valid way or they might not, so I think having DECL_BUILT_IN_CLASS other
> than BUILT_IN_NORMAL is right.
> 
> The problem is that we don't do proper checking.
> 
> In generic-match.cc, we just call get_call_combined_fn which performs no
> checking of the arguments whatsoever.
> 
> In gimple-match.cc, we call gimple_call_combined_fn, which performs bad
> checking of the arguments.
> 
> When not an internal function (which doesn't need argument checking, the
> compiler guarantees it is right), the former does just:
>   tree fndecl = get_callee_fndecl (call);
>   if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
>     return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> In GIMPLE, we call:
>   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> but that is insufficient, that verifies whether the arguments passed to
> GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> be the user declaration, so doesn't have to match exactly the builtin
> as predeclared by builtins.def etc. (though, there is the cotcha that say
> for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> we use additional:
>   tree callee = gimple_call_fndecl (stmt);
>   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
>   if (decl
>       && decl != callee
>       && !gimple_builtin_call_types_compatible_p (stmt, decl))
>     return false;

Yeah, I think we should use that (and only that) function decl
in get_call_combined_fn and gimple_call_combined_fn until the
frontend stops slapping wrong BUILT_IN_* on random decls.

> For this particular testcase, seems the problem is just the generic folding,
> if I add && GIMPLE to the sqrt comparison foldings, it doesn't ICE anymore,
> so just adding generic_builtin_call_types_compatible_p (basically copy
> gimple_builtin_call_types_compatible_p) might be enough.
> Though a big question is what it should do, using useless_type_conversion_p
> might be needed so that it can deal e.g. with the FILE * vs. void *
> differences (and various others), on the other side, what e.g. the exact
> sqrt comparison folding requires to be much more strict, as it turns
> (cmp (sq @0) REAL_CST@1)
> to
> (cmp @0 @1)
> and similarly for
> (cmp (sq @0) (sq @1))
> and that requires exact type match, no?

Yes, on GENERIC it requires types_match (), so get_call_combined_fn
should use a type checking variant that checks that instead of
useless_type_conversion_p.

> So one possibility would be to use useless_type_conversion_p but actually
> not write (cmp @0 @1) in such cases, but (cmp (convert:op1type @0) @1)
> or so?

No, I think the match.pd patterns are all fine and the fix has to be
elsewhere.  Either in the generic get_call_combined_fn or in
extra plumbing emitted by genmatch (I prefer to fix get_call_combined_fn
and friends since we already do some checking for GIMPLE).  Well,
of course I prefer to have the frontend fixed, but ...

Richard.

[PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Jakub Jelinek]

On Tue, Apr 05, 2022 at 11:28:53AM +0200, Richard Biener wrote:
> > In GIMPLE, we call:
> >   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> > but that is insufficient, that verifies whether the arguments passed to
> > GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> > be the user declaration, so doesn't have to match exactly the builtin
> > as predeclared by builtins.def etc. (though, there is the cotcha that say
> > for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> > we use additional:
> >   tree callee = gimple_call_fndecl (stmt);
> >   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
> >   if (decl
> >       && decl != callee
> >       && !gimple_builtin_call_types_compatible_p (stmt, decl))
> >     return false;
> 
> Yeah, I think we should use that (and only that) function decl
> in get_call_combined_fn and gimple_call_combined_fn until the
> frontend stops slapping wrong BUILT_IN_* on random decls.

So, as a preparation step, this patch adjusts gimple_call_builtin_p
and gimple_call_combined_fn so that they check argument types against
the builtin_decl_explicit TYPE_ARG_TYPES rather than against the
actual used fndecl.

Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

Will work on generic tree_builtin_call_types_compatible_p next.

	Jakub

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Biener]

On Tue, 5 Apr 2022, Jakub Jelinek wrote:

> On Tue, Apr 05, 2022 at 11:28:53AM +0200, Richard Biener wrote:
> > > In GIMPLE, we call:
> > >   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> > > but that is insufficient, that verifies whether the arguments passed to
> > > GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> > > be the user declaration, so doesn't have to match exactly the builtin
> > > as predeclared by builtins.def etc. (though, there is the cotcha that say
> > > for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> > > we use additional:
> > >   tree callee = gimple_call_fndecl (stmt);
> > >   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
> > >   if (decl
> > >       && decl != callee
> > >       && !gimple_builtin_call_types_compatible_p (stmt, decl))
> > >     return false;
> > 
> > Yeah, I think we should use that (and only that) function decl
> > in get_call_combined_fn and gimple_call_combined_fn until the
> > frontend stops slapping wrong BUILT_IN_* on random decls.
> 
> So, as a preparation step, this patch adjusts gimple_call_builtin_p
> and gimple_call_combined_fn so that they check argument types against
> the builtin_decl_explicit TYPE_ARG_TYPES rather than against the
> actual used fndecl.
> 
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

You forgot to attach the patch ...

Richard.

> Will work on generic tree_builtin_call_types_compatible_p next.
> 
> 	Jakub

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Jakub Jelinek]

On Wed, Apr 06, 2022 at 08:13:24AM +0200, Richard Biener wrote:
> On Tue, 5 Apr 2022, Jakub Jelinek wrote:
> 
> > On Tue, Apr 05, 2022 at 11:28:53AM +0200, Richard Biener wrote:
> > > > In GIMPLE, we call:
> > > >   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> > > > but that is insufficient, that verifies whether the arguments passed to
> > > > GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> > > > be the user declaration, so doesn't have to match exactly the builtin
> > > > as predeclared by builtins.def etc. (though, there is the cotcha that say
> > > > for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> > > > we use additional:
> > > >   tree callee = gimple_call_fndecl (stmt);
> > > >   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
> > > >   if (decl
> > > >       && decl != callee
> > > >       && !gimple_builtin_call_types_compatible_p (stmt, decl))
> > > >     return false;
> > > 
> > > Yeah, I think we should use that (and only that) function decl
> > > in get_call_combined_fn and gimple_call_combined_fn until the
> > > frontend stops slapping wrong BUILT_IN_* on random decls.
> > 
> > So, as a preparation step, this patch adjusts gimple_call_builtin_p
> > and gimple_call_combined_fn so that they check argument types against
> > the builtin_decl_explicit TYPE_ARG_TYPES rather than against the
> > actual used fndecl.
> > 
> > Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
> 
> You forgot to attach the patch ...

Oops, here it is.  Also bootstrapped/regtested on {x86_64,i686}-linux.

2022-04-06  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/105150
	* gimple.cc (gimple_call_builtin_p, gimple_call_combined_fn):
	For BUILT_IN_NORMAL calls, call gimple_builtin_call_types_compatible_p
	preferrably on builtin_decl_explicit decl rather than fndecl.
	* tree-ssa-strlen.cc (valid_builtin_call): Don't call
	gimple_builtin_call_types_compatible_p here.

--- gcc/gimple.cc.jj	2022-02-09 15:15:59.436837973 +0100
+++ gcc/gimple.cc	2022-04-05 13:04:58.405586995 +0200
@@ -2841,7 +2841,12 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
-    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    {
+      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
+	  fndecl = decl;
+      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    }
   return false;
 }
 
@@ -2854,7 +2859,12 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && DECL_BUILT_IN_CLASS (fndecl) == klass)
-    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    {
+      if (klass == BUILT_IN_NORMAL)
+	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
+	  fndecl = decl;
+      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    }
   return false;
 }
 
@@ -2867,7 +2877,11 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && fndecl_built_in_p (fndecl, code))
-    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    {
+      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
+	fndecl = decl;
+      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
+    }
   return false;
 }
 
@@ -2884,10 +2898,14 @@ gimple_call_combined_fn (const gimple *s
 	return as_combined_fn (gimple_call_internal_fn (call));
 
       tree fndecl = gimple_call_fndecl (stmt);
-      if (fndecl
-	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
-	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
-	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
+      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
+	{
+	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
+	  if (!decl)
+	    decl = fndecl;
+	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
+	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
+	}
     }
   return CFN_LAST;
 }
--- gcc/tree-ssa-strlen.cc.jj	2022-03-30 09:11:53.310103911 +0200
+++ gcc/tree-ssa-strlen.cc	2022-04-05 12:22:56.023057416 +0200
@@ -1736,12 +1736,6 @@ valid_builtin_call (gimple *stmt)
     return false;
 
   tree callee = gimple_call_fndecl (stmt);
-  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
-  if (decl
-      && decl != callee
-      && !gimple_builtin_call_types_compatible_p (stmt, decl))
-    return false;
-
   switch (DECL_FUNCTION_CODE (callee))
     {
     case BUILT_IN_MEMCMP:


	Jakub

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Biener]

On Wed, 6 Apr 2022, Jakub Jelinek wrote:

> On Wed, Apr 06, 2022 at 08:13:24AM +0200, Richard Biener wrote:
> > On Tue, 5 Apr 2022, Jakub Jelinek wrote:
> > 
> > > On Tue, Apr 05, 2022 at 11:28:53AM +0200, Richard Biener wrote:
> > > > > In GIMPLE, we call:
> > > > >   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
> > > > > but that is insufficient, that verifies whether the arguments passed to
> > > > > GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
> > > > > be the user declaration, so doesn't have to match exactly the builtin
> > > > > as predeclared by builtins.def etc. (though, there is the cotcha that say
> > > > > for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
> > > > > we use additional:
> > > > >   tree callee = gimple_call_fndecl (stmt);
> > > > >   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
> > > > >   if (decl
> > > > >       && decl != callee
> > > > >       && !gimple_builtin_call_types_compatible_p (stmt, decl))
> > > > >     return false;
> > > > 
> > > > Yeah, I think we should use that (and only that) function decl
> > > > in get_call_combined_fn and gimple_call_combined_fn until the
> > > > frontend stops slapping wrong BUILT_IN_* on random decls.
> > > 
> > > So, as a preparation step, this patch adjusts gimple_call_builtin_p
> > > and gimple_call_combined_fn so that they check argument types against
> > > the builtin_decl_explicit TYPE_ARG_TYPES rather than against the
> > > actual used fndecl.
> > > 
> > > Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
> > 
> > You forgot to attach the patch ...
> 
> Oops, here it is.  Also bootstrapped/regtested on {x86_64,i686}-linux.

OK.

Thanks,
Richard.

> 2022-04-06  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/105150
> 	* gimple.cc (gimple_call_builtin_p, gimple_call_combined_fn):
> 	For BUILT_IN_NORMAL calls, call gimple_builtin_call_types_compatible_p
> 	preferrably on builtin_decl_explicit decl rather than fndecl.
> 	* tree-ssa-strlen.cc (valid_builtin_call): Don't call
> 	gimple_builtin_call_types_compatible_p here.
> 
> --- gcc/gimple.cc.jj	2022-02-09 15:15:59.436837973 +0100
> +++ gcc/gimple.cc	2022-04-05 13:04:58.405586995 +0200
> @@ -2841,7 +2841,12 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    {
> +      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> +	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> +	  fndecl = decl;
> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    }
>    return false;
>  }
>  
> @@ -2854,7 +2859,12 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) == klass)
> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    {
> +      if (klass == BUILT_IN_NORMAL)
> +	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> +	  fndecl = decl;
> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    }
>    return false;
>  }
>  
> @@ -2867,7 +2877,11 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && fndecl_built_in_p (fndecl, code))
> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    {
> +      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> +	fndecl = decl;
> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> +    }
>    return false;
>  }
>  
> @@ -2884,10 +2898,14 @@ gimple_call_combined_fn (const gimple *s
>  	return as_combined_fn (gimple_call_internal_fn (call));
>  
>        tree fndecl = gimple_call_fndecl (stmt);
> -      if (fndecl
> -	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
> -	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
> -	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> +      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
> +	{
> +	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
> +	  if (!decl)
> +	    decl = fndecl;
> +	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
> +	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> +	}
>      }
>    return CFN_LAST;
>  }
> --- gcc/tree-ssa-strlen.cc.jj	2022-03-30 09:11:53.310103911 +0200
> +++ gcc/tree-ssa-strlen.cc	2022-04-05 12:22:56.023057416 +0200
> @@ -1736,12 +1736,6 @@ valid_builtin_call (gimple *stmt)
>      return false;
>  
>    tree callee = gimple_call_fndecl (stmt);
> -  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
> -  if (decl
> -      && decl != callee
> -      && !gimple_builtin_call_types_compatible_p (stmt, decl))
> -    return false;
> -
>    switch (DECL_FUNCTION_CODE (callee))
>      {
>      case BUILT_IN_MEMCMP:
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Sandiford]

Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Wed, 6 Apr 2022, Jakub Jelinek wrote:
>
>> On Wed, Apr 06, 2022 at 08:13:24AM +0200, Richard Biener wrote:
>> > On Tue, 5 Apr 2022, Jakub Jelinek wrote:
>> > 
>> > > On Tue, Apr 05, 2022 at 11:28:53AM +0200, Richard Biener wrote:
>> > > > > In GIMPLE, we call:
>> > > > >   && gimple_builtin_call_types_compatible_p (stmt, fndecl)
>> > > > > but that is insufficient, that verifies whether the arguments passed to
>> > > > > GIMPLE_CALL match the fndecl argument types.  But that fndecl may very well
>> > > > > be the user declaration, so doesn't have to match exactly the builtin
>> > > > > as predeclared by builtins.def etc. (though, there is the cotcha that say
>> > > > > for FILE * we just use void * etc.).  So e.g. in tree-ssa-strlen.cc
>> > > > > we use additional:
>> > > > >   tree callee = gimple_call_fndecl (stmt);
>> > > > >   tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
>> > > > >   if (decl
>> > > > >       && decl != callee
>> > > > >       && !gimple_builtin_call_types_compatible_p (stmt, decl))
>> > > > >     return false;
>> > > > 
>> > > > Yeah, I think we should use that (and only that) function decl
>> > > > in get_call_combined_fn and gimple_call_combined_fn until the
>> > > > frontend stops slapping wrong BUILT_IN_* on random decls.
>> > > 
>> > > So, as a preparation step, this patch adjusts gimple_call_builtin_p
>> > > and gimple_call_combined_fn so that they check argument types against
>> > > the builtin_decl_explicit TYPE_ARG_TYPES rather than against the
>> > > actual used fndecl.
>> > > 
>> > > Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
>> > 
>> > You forgot to attach the patch ...
>> 
>> Oops, here it is.  Also bootstrapped/regtested on {x86_64,i686}-linux.
>
> OK.

But it seems like the magic incantation to detect “real” built-in
function calls is getting longer and longer.  Can we not abstract this
in a single place rather than have to repeat the same long sequence in
multiple places?

Thanks,
Richard

>
> Thanks,
> Richard.
>
>> 2022-04-06  Jakub Jelinek  <jakub@redhat.com>
>> 
>> 	PR tree-optimization/105150
>> 	* gimple.cc (gimple_call_builtin_p, gimple_call_combined_fn):
>> 	For BUILT_IN_NORMAL calls, call gimple_builtin_call_types_compatible_p
>> 	preferrably on builtin_decl_explicit decl rather than fndecl.
>> 	* tree-ssa-strlen.cc (valid_builtin_call): Don't call
>> 	gimple_builtin_call_types_compatible_p here.
>> 
>> --- gcc/gimple.cc.jj	2022-02-09 15:15:59.436837973 +0100
>> +++ gcc/gimple.cc	2022-04-05 13:04:58.405586995 +0200
>> @@ -2841,7 +2841,12 @@ gimple_call_builtin_p (const gimple *stm
>>    if (is_gimple_call (stmt)
>>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>>        && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
>> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    {
>> +      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>> +	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
>> +	  fndecl = decl;
>> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    }
>>    return false;
>>  }
>>  
>> @@ -2854,7 +2859,12 @@ gimple_call_builtin_p (const gimple *stm
>>    if (is_gimple_call (stmt)
>>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>>        && DECL_BUILT_IN_CLASS (fndecl) == klass)
>> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    {
>> +      if (klass == BUILT_IN_NORMAL)
>> +	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
>> +	  fndecl = decl;
>> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    }
>>    return false;
>>  }
>>  
>> @@ -2867,7 +2877,11 @@ gimple_call_builtin_p (const gimple *stm
>>    if (is_gimple_call (stmt)
>>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>>        && fndecl_built_in_p (fndecl, code))
>> -    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    {
>> +      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
>> +	fndecl = decl;
>> +      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>> +    }
>>    return false;
>>  }
>>  
>> @@ -2884,10 +2898,14 @@ gimple_call_combined_fn (const gimple *s
>>  	return as_combined_fn (gimple_call_internal_fn (call));
>>  
>>        tree fndecl = gimple_call_fndecl (stmt);
>> -      if (fndecl
>> -	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
>> -	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
>> -	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
>> +      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
>> +	{
>> +	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
>> +	  if (!decl)
>> +	    decl = fndecl;
>> +	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
>> +	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
>> +	}
>>      }
>>    return CFN_LAST;
>>  }
>> --- gcc/tree-ssa-strlen.cc.jj	2022-03-30 09:11:53.310103911 +0200
>> +++ gcc/tree-ssa-strlen.cc	2022-04-05 12:22:56.023057416 +0200
>> @@ -1736,12 +1736,6 @@ valid_builtin_call (gimple *stmt)
>>      return false;
>>  
>>    tree callee = gimple_call_fndecl (stmt);
>> -  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
>> -  if (decl
>> -      && decl != callee
>> -      && !gimple_builtin_call_types_compatible_p (stmt, decl))
>> -    return false;
>> -
>>    switch (DECL_FUNCTION_CODE (callee))
>>      {
>>      case BUILT_IN_MEMCMP:
>> 
>> 
>> 	Jakub
>> 
>> 

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Jakub Jelinek]

On Wed, Apr 06, 2022 at 09:41:44AM +0100, Richard Sandiford wrote:
> But it seems like the magic incantation to detect “real” built-in
> function calls is getting longer and longer.  Can we not abstract this
> in a single place rather than have to repeat the same long sequence in
> multiple places?

I've already committed it, so it can be only dealt with an incremental
patch.
One possibility is to do it inside of
gimple_builtin_call_types_compatible_p, after the assert do that:
  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
    if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
      fndecl = decl;
but we then lose the theoretical possibility of comparing against the
actual user declaration.  Though I guess in the
gimple-fold.cc
gimple-low.cc
gimple-match-head.cc
calls to that function we also want this rather than what they do currently.

	Jakub

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Biener]

On Wed, 6 Apr 2022, Jakub Jelinek wrote:

> On Wed, Apr 06, 2022 at 09:41:44AM +0100, Richard Sandiford wrote:
> > But it seems like the magic incantation to detect “real” built-in
> > function calls is getting longer and longer.  Can we not abstract this
> > in a single place rather than have to repeat the same long sequence in
> > multiple places?
> 
> I've already committed it, so it can be only dealt with an incremental
> patch.
> One possibility is to do it inside of
> gimple_builtin_call_types_compatible_p, after the assert do that:
>   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>     if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
>       fndecl = decl;
> but we then lose the theoretical possibility of comparing against the
> actual user declaration.  Though I guess in the
> gimple-fold.cc
> gimple-low.cc
> gimple-match-head.cc
> calls to that function we also want this rather than what they do currently.

Yes, I think it would be clearer to pass a BUILT_IN_* code to
gimple_builtin_call_types_compatible_p and no decl and simply return
false if we cannot get out hands at the "proper" decl from
builtin_decl_explicit ...

Richard.

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Jakub Jelinek]

On Wed, Apr 06, 2022 at 11:52:23AM +0200, Richard Biener wrote:
> On Wed, 6 Apr 2022, Jakub Jelinek wrote:
> 
> > On Wed, Apr 06, 2022 at 09:41:44AM +0100, Richard Sandiford wrote:
> > > But it seems like the magic incantation to detect “real” built-in
> > > function calls is getting longer and longer.  Can we not abstract this
> > > in a single place rather than have to repeat the same long sequence in
> > > multiple places?
> > 
> > I've already committed it, so it can be only dealt with an incremental
> > patch.
> > One possibility is to do it inside of
> > gimple_builtin_call_types_compatible_p, after the assert do that:
> >   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> >     if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> >       fndecl = decl;
> > but we then lose the theoretical possibility of comparing against the
> > actual user declaration.  Though I guess in the
> > gimple-fold.cc
> > gimple-low.cc
> > gimple-match-head.cc
> > calls to that function we also want this rather than what they do currently.
> 
> Yes, I think it would be clearer to pass a BUILT_IN_* code to
> gimple_builtin_call_types_compatible_p and no decl and simply return
> false if we cannot get out hands at the "proper" decl from
> builtin_decl_explicit ...

That would mean we wouldn't verify the md or FE builtins anymore
and we would need to check for BUILT_IN_NORMAL in every caller (right now
we do that only in some of them).

Here is what I had in mind (untested so far):

2022-04-06  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/105150
	* gimple.cc (gimple_builtin_call_types_compatible_p): Use
	builtin_decl_explicit here...
	(gimple_call_builtin_p, gimple_call_combined_fn): ... rather than
	here.

--- gcc/gimple.cc.jj	2022-04-06 10:07:23.043064595 +0200
+++ gcc/gimple.cc	2022-04-06 11:31:31.704255242 +0200
@@ -2788,6 +2788,10 @@ gimple_builtin_call_types_compatible_p (
 {
   gcc_checking_assert (DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN);
 
+  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+    if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
+      fndecl = decl;
+
   tree ret = gimple_call_lhs (stmt);
   if (ret
       && !useless_type_conversion_p (TREE_TYPE (ret),
@@ -2841,12 +2845,7 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
-    {
-      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
-	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
-	  fndecl = decl;
-      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
-    }
+    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
   return false;
 }
 
@@ -2859,12 +2858,7 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && DECL_BUILT_IN_CLASS (fndecl) == klass)
-    {
-      if (klass == BUILT_IN_NORMAL)
-	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
-	  fndecl = decl;
-      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
-    }
+    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
   return false;
 }
 
@@ -2877,11 +2871,7 @@ gimple_call_builtin_p (const gimple *stm
   if (is_gimple_call (stmt)
       && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
       && fndecl_built_in_p (fndecl, code))
-    {
-      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
-	fndecl = decl;
-      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
-    }
+    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
   return false;
 }
 
@@ -2898,14 +2888,10 @@ gimple_call_combined_fn (const gimple *s
 	return as_combined_fn (gimple_call_internal_fn (call));
 
       tree fndecl = gimple_call_fndecl (stmt);
-      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
-	{
-	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
-	  if (!decl)
-	    decl = fndecl;
-	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
-	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
-	}
+      if (fndecl
+	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
+	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
+	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
     }
   return CFN_LAST;
 }


	Jakub

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Biener]

On Wed, 6 Apr 2022, Jakub Jelinek wrote:

> On Wed, Apr 06, 2022 at 11:52:23AM +0200, Richard Biener wrote:
> > On Wed, 6 Apr 2022, Jakub Jelinek wrote:
> > 
> > > On Wed, Apr 06, 2022 at 09:41:44AM +0100, Richard Sandiford wrote:
> > > > But it seems like the magic incantation to detect “real” built-in
> > > > function calls is getting longer and longer.  Can we not abstract this
> > > > in a single place rather than have to repeat the same long sequence in
> > > > multiple places?
> > > 
> > > I've already committed it, so it can be only dealt with an incremental
> > > patch.
> > > One possibility is to do it inside of
> > > gimple_builtin_call_types_compatible_p, after the assert do that:
> > >   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> > >     if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> > >       fndecl = decl;
> > > but we then lose the theoretical possibility of comparing against the
> > > actual user declaration.  Though I guess in the
> > > gimple-fold.cc
> > > gimple-low.cc
> > > gimple-match-head.cc
> > > calls to that function we also want this rather than what they do currently.
> > 
> > Yes, I think it would be clearer to pass a BUILT_IN_* code to
> > gimple_builtin_call_types_compatible_p and no decl and simply return
> > false if we cannot get out hands at the "proper" decl from
> > builtin_decl_explicit ...
> 
> That would mean we wouldn't verify the md or FE builtins anymore
> and we would need to check for BUILT_IN_NORMAL in every caller (right now
> we do that only in some of them).
> 
> Here is what I had in mind (untested so far):

Yes, that works as well for me.

Richard.

> 2022-04-06  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/105150
> 	* gimple.cc (gimple_builtin_call_types_compatible_p): Use
> 	builtin_decl_explicit here...
> 	(gimple_call_builtin_p, gimple_call_combined_fn): ... rather than
> 	here.
> 
> --- gcc/gimple.cc.jj	2022-04-06 10:07:23.043064595 +0200
> +++ gcc/gimple.cc	2022-04-06 11:31:31.704255242 +0200
> @@ -2788,6 +2788,10 @@ gimple_builtin_call_types_compatible_p (
>  {
>    gcc_checking_assert (DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN);
>  
> +  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> +    if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> +      fndecl = decl;
> +
>    tree ret = gimple_call_lhs (stmt);
>    if (ret
>        && !useless_type_conversion_p (TREE_TYPE (ret),
> @@ -2841,12 +2845,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
> -    {
> -      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> -	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	  fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2859,12 +2858,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) == klass)
> -    {
> -      if (klass == BUILT_IN_NORMAL)
> -	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	  fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2877,11 +2871,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && fndecl_built_in_p (fndecl, code))
> -    {
> -      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2898,14 +2888,10 @@ gimple_call_combined_fn (const gimple *s
>  	return as_combined_fn (gimple_call_internal_fn (call));
>  
>        tree fndecl = gimple_call_fndecl (stmt);
> -      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
> -	{
> -	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
> -	  if (!decl)
> -	    decl = fndecl;
> -	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
> -	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> -	}
> +      if (fndecl
> +	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
> +	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
> +	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
>      }
>    return CFN_LAST;
>  }
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)

Re: [PATCH] gimple.cc: Adjust gimple_call_builtin_p and gimple_call_combined_fn [PR105150]

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Apr 06, 2022 at 11:52:23AM +0200, Richard Biener wrote:
>> On Wed, 6 Apr 2022, Jakub Jelinek wrote:
>> 
>> > On Wed, Apr 06, 2022 at 09:41:44AM +0100, Richard Sandiford wrote:
>> > > But it seems like the magic incantation to detect “real” built-in
>> > > function calls is getting longer and longer.  Can we not abstract this
>> > > in a single place rather than have to repeat the same long sequence in
>> > > multiple places?
>> > 
>> > I've already committed it, so it can be only dealt with an incremental
>> > patch.
>> > One possibility is to do it inside of
>> > gimple_builtin_call_types_compatible_p, after the assert do that:
>> >   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>> >     if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
>> >       fndecl = decl;
>> > but we then lose the theoretical possibility of comparing against the
>> > actual user declaration.  Though I guess in the
>> > gimple-fold.cc
>> > gimple-low.cc
>> > gimple-match-head.cc
>> > calls to that function we also want this rather than what they do currently.
>> 
>> Yes, I think it would be clearer to pass a BUILT_IN_* code to
>> gimple_builtin_call_types_compatible_p and no decl and simply return
>> false if we cannot get out hands at the "proper" decl from
>> builtin_decl_explicit ...
>
> That would mean we wouldn't verify the md or FE builtins anymore
> and we would need to check for BUILT_IN_NORMAL in every caller (right now
> we do that only in some of them).
>
> Here is what I had in mind (untested so far):
>
> 2022-04-06  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/105150
> 	* gimple.cc (gimple_builtin_call_types_compatible_p): Use
> 	builtin_decl_explicit here...
> 	(gimple_call_builtin_p, gimple_call_combined_fn): ... rather than
> 	here.

Nice!  Thanks for doing this.

Richard

> --- gcc/gimple.cc.jj	2022-04-06 10:07:23.043064595 +0200
> +++ gcc/gimple.cc	2022-04-06 11:31:31.704255242 +0200
> @@ -2788,6 +2788,10 @@ gimple_builtin_call_types_compatible_p (
>  {
>    gcc_checking_assert (DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN);
>  
> +  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> +    if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> +      fndecl = decl;
> +
>    tree ret = gimple_call_lhs (stmt);
>    if (ret
>        && !useless_type_conversion_p (TREE_TYPE (ret),
> @@ -2841,12 +2845,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN)
> -    {
> -      if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> -	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	  fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2859,12 +2858,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && DECL_BUILT_IN_CLASS (fndecl) == klass)
> -    {
> -      if (klass == BUILT_IN_NORMAL)
> -	if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	  fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2877,11 +2871,7 @@ gimple_call_builtin_p (const gimple *stm
>    if (is_gimple_call (stmt)
>        && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE
>        && fndecl_built_in_p (fndecl, code))
> -    {
> -      if (tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl)))
> -	fndecl = decl;
> -      return gimple_builtin_call_types_compatible_p (stmt, fndecl);
> -    }
> +    return gimple_builtin_call_types_compatible_p (stmt, fndecl);
>    return false;
>  }
>  
> @@ -2898,14 +2888,10 @@ gimple_call_combined_fn (const gimple *s
>  	return as_combined_fn (gimple_call_internal_fn (call));
>  
>        tree fndecl = gimple_call_fndecl (stmt);
> -      if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
> -	{
> -	  tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl));
> -	  if (!decl)
> -	    decl = fndecl;
> -	  if (gimple_builtin_call_types_compatible_p (stmt, decl))
> -	    return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
> -	}
> +      if (fndecl
> +	  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
> +	  && gimple_builtin_call_types_compatible_p (stmt, fndecl))
> +	return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
>      }
>    return CFN_LAST;
>  }
>
>
> 	Jakub