Re: [PATCH] rs6000: Fix 2 for PR92661, Do not define builtins that overload disabled builtins

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

[ This email was refused by the ML, too big, but I'll keep all non-
mechanical parts of the patch in this reply, for the archives. ]

On Wed, Dec 04, 2019 at 10:01:05AM -0600, Peter Bergner wrote:
> The following patch fixes the last bug in PR92661, which is an ICE while
> defining a builtin that overloads another builtin which doesn't exist
> (because it was disabled, etc.).  The fix here is to check that the builtin
> we are overloading has been defined before we allow another builtin to
> overload it.

> I have also
> included a small patch to disable running the powerpc/dfp/ tests even for
> powerpc*-linux when --disable-decimal-float is used.

What is the reason for that?

> I also renamed dfp.exp
> to powerpc-dfp.exp as we discussed offline to make it easier for us to run.

Yup.  The problem was that there were two dfp.exp for check-gcc-c for us.

> gcc/
> 	PR bootstrap/92661
> 	* config/rs6000/rs6000-c.c (struct altivec_builtin_types): Move to
> 	rs6000.h.
> 	(altivec_overloaded_builtins): Move to rs6000-call.c.
> 	* config/rs6000/rs6000.h (struct altivec_builtin_types): Moved from
> 	rs6000-c.c.
> 	* config/rs6000/rs6000-call.c (rs6000_builtin_info): Make static.
> 	(altivec_overloaded_builtins): Moved from rs6000-c.c.
> 	(rs6000_common_init_builtins): Do no define builtins that overload
> 	builtins that have been disabled.
> 
> gcc/testsuite/
> 	PR bootstrap/92661
> 	* gcc.target/powerpc/dfp/dfp.exp: Rename from this...
> 	* gcc.target/powerpc/dfp/powerpc-dfp.exp: ...to this.
> 	Use check_effective_target_dfp.

For future patches: it is much easier to review if you make the big,
mechanical move a separate (earlier) patch.

> --- gcc/config/rs6000/rs6000-call.c	(revision 278946)
> +++ gcc/config/rs6000/rs6000-call.c	(working copy)
> @@ -276,7 +276,7 @@ struct rs6000_builtin_info_type {
>    const unsigned attr;
>  };
>  
> -const struct rs6000_builtin_info_type rs6000_builtin_info[] =
> +static const struct rs6000_builtin_info_type rs6000_builtin_info[] =
>  {
>  #include "rs6000-builtin.def"
>  };

> @@ -7844,12 +13048,23 @@ rs6000_common_init_builtins (void)
>  
>        if (rs6000_overloaded_builtin_p (d->code))
>  	{
> -	  if (! (type = opaque_ftype_opaque_opaque))
> -	    type = opaque_ftype_opaque_opaque
> -	      = build_function_type_list (opaque_V4SI_type_node,
> -					  opaque_V4SI_type_node,
> -					  opaque_V4SI_type_node,
> -					  NULL_TREE);
> +	  const struct altivec_builtin_types *desc;
> +
> +	  /* Verify the builtin we are overloading has already been defined.  */
> +	  type = NULL_TREE;
> +	  for (desc = altivec_overloaded_builtins;
> +	       desc->code != RS6000_BUILTIN_NONE; desc++)
> +	    if (desc->code == d->code
> +		&& rs6000_builtin_decls[(int)desc->overloaded_code])
> +	      {
> +		if (! (type = opaque_ftype_opaque_opaque))
> +		  type = opaque_ftype_opaque_opaque
> +		    = build_function_type_list (opaque_V4SI_type_node,
> +						opaque_V4SI_type_node,
> +						opaque_V4SI_type_node,
> +						NULL_TREE);
> +		break;
> +	      }
>  	}
>        else
>  	{

> --- gcc/config/rs6000/rs6000.h	(revision 278946)
> +++ gcc/config/rs6000/rs6000.h	(working copy)
> @@ -2365,6 +2365,18 @@ enum rs6000_builtins
>  #undef RS6000_BUILTIN_P
>  #undef RS6000_BUILTIN_X
>  
> +/* Mappings for overloaded builtins.  */
> +struct altivec_builtin_types
> +{
> +  enum rs6000_builtins code;
> +  enum rs6000_builtins overloaded_code;
> +  signed char ret_type;
> +  signed char op1;
> +  signed char op2;
> +  signed char op3;
> +};
> +extern const struct altivec_builtin_types altivec_overloaded_builtins[];
> +
>  enum rs6000_builtin_type_index
>  {
>    RS6000_BTI_NOT_OPAQUE,

> --- gcc/testsuite/gcc.target/powerpc/dfp/powerpc-dfp.exp	(revision 278946)
> +++ gcc/testsuite/gcc.target/powerpc/dfp/powerpc-dfp.exp	(working copy)
> @@ -16,12 +16,9 @@
>  # along with GCC; see the file COPYING3.  If not see
>  # <http://www.gnu.org/licenses/>.
>  
> -# Exit immediately if this isn't a PowerPC target, also exit if we
> -# are on Darwin which doesn't support decimal float.
> -if { (![istarget powerpc*-*-*] && ![istarget rs6000-*-*])
> -    || [istarget "powerpc*-*-darwin*"]
> -} then {
> -  return
> +# Skip these tests for targets that don't support this extension.
> +if { ![check_effective_target_dfp] } {
> +    return;
>  }
>  
>  global DEFAULT_CFLAGS

This isn't run from powerpc.exp, so it needs to still do that first check.
And it's up to the Darwin maintainers whether they want that second part
(there are many more tests and testsuites that disable *-darwin* while
that isn't really necessary).

Why do you need/want the check_effective_target_dfp test?  If for example
this is to prevent ICEs, that is no good, that is *hiding* problems.

I suspect it is to stop the testsuite from complaining if you configure
with --disable-decimal-float.  What is different there then, compared to
targets that do actually not support decimal float?

Okay for trunk minus the changes to powerpc-dfp.exp (we can iterate on
that).  Thanks!


Segher