[1/3] Rework 128-bit complex multiply and divide, PR target/107299

  This function reworks how the complex multiply and divide built-in functions are
done.  Previously we created built-in declarations for doing long double complex
multiply and divide when long double is IEEE 128-bit.  The old code also did not
support __ibm128 complex multiply and divide if long double is IEEE 128-bit.

In terms of history, I wrote the original code just as I was starting to test
GCC on systems where IEEE 128-bit long double was the default.  At the time, we
had not yet started mangling the built-in function names as a way to bridge
going from a system with 128-bit IBM long double to 128-bin IEEE long double.

The original code depends on there only being two 128-bit types invovled.  With
the next patch in this series, this assumption will no longer be true.  When
long double is IEEE 128-bit, there will be 2 IEEE 128-bit types (one for the
explicit __float128/_Float128 type and one for long double).

The problem is we cannot create two separate built-in functions that resolve to
the same name.  This is a requirement of add_builtin_function and the C front
end.  That means for the 3 possible modes (IFmode, KFmode, and TFmode), you can
only use 2 of them.

This code does not create the built-in declaration with the changed name.
Instead, it uses the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change the name
before it is written out to the assembler file like it now does for all of the
other long double built-in functions.

We need to disable using this mapping when we are building libgcc, specifically
when it is building the floating point 128-bit multiply and divide functions.
The flag that is used when libgcc is built (-fbuilding-libcc) is only available
in the C/C++ front ends.  We need to remember that we are building libgcc in the
rs6000-c.cc support to be able to use this later to decided whether to mangle
the decl assembler name or not.

When I wrote these patches, I discovered that __ibm128 complex multiply and
divide had originally not been supported if long double is IEEE 128-bit as it
would generate calls to __mulic3 and __divic3.  I added tests in the testsuite
to verify that the correct name (i.e. __multc3 and __divtc3) is used in this
case.

I tested all 3 patchs for PR target/107299 on:

    1)	LE Power10 using --with-cpu=power10 --with-long-double-format=ieee
    2)	LE Power10 using --with-cpu=power10 --with-long-double-format=ibm
    3)	LE Power9  using --with-cpu=power9  --with-long-double-format=ibm
    4)	BE Power8  using --with-cpu=power8  --with-long-double-format=ibm

Once all 3 patches have been applied, we can once again build GCC when long
double is IEEE 128-bit.  There were no other regressions with these patches.
Can I check these patches into the trunk?

2022-11-01   Michael Meissner  <meissner@linux.ibm.com>

gcc/

	PR target/107299
	* config/rs6000/rs6000-c.cc (rs6000_cpu_cpp_builtins): Set
	building_libgcc.
	* config/rs6000/rs6000.cc (create_complex_muldiv): Delete.
	(init_float128_ieee): Delete code to switch complex multiply and divide
	for long double.
	(complex_multiply_builtin_code): New helper function.
	(complex_divide_builtin_code): Likewise.
	(rs6000_mangle_decl_assembler_name): Add support for mangling the name
	of complex 128-bit multiply and divide built-in functions.
	* config/rs6000/rs6000.opt (building_libgcc): New target variable.

gcc/testsuite/

	PR target/107299
	* gcc.target/powerpc/divic3-1.c: New test.
	* gcc.target/powerpc/divic3-2.c: Likewise.
	* gcc.target/powerpc/mulic3-1.c: Likewise.
	* gcc.target/powerpc/mulic3-2.c: Likewise.
---
 gcc/config/rs6000/rs6000-c.cc               |   8 ++
 gcc/config/rs6000/rs6000.cc                 | 110 +++++++++++---------
 gcc/config/rs6000/rs6000.opt                |   4 +
 gcc/testsuite/gcc.target/powerpc/divic3-1.c |  18 ++++
 gcc/testsuite/gcc.target/powerpc/divic3-2.c |  17 +++
 gcc/testsuite/gcc.target/powerpc/mulic3-1.c |  18 ++++
 gcc/testsuite/gcc.target/powerpc/mulic3-2.c |  17 +++
 7 files changed, 145 insertions(+), 47 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/divic3-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/divic3-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/mulic3-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/mulic3-2.c
  

Ping patch:

| Date: Tue, 1 Nov 2022 22:40:43 -0400
| Subject: [PATCH 1/3] Rework 128-bit complex multiply and divide, PR target/107299
| Message-ID: <Y2HYqx4zLCNCT0Zy@toto.the-meissners.org>

This patch is needed to build GCC on Fedora 36 where the default for long
double is now IEEE 128-bit.
  

Can we please get this patch reviewed?  GCC 13 won't build on Fedora 37 (which
defaults to long double being IEEE 128-bit) without the 3 patches in this set.

| Date: Tue, 1 Nov 2022 22:40:43 -0400
| Subject: [PATCH 1/3] Rework 128-bit complex multiply and divide, PR target/107299
| Message-ID: <Y2HYqx4zLCNCT0Zy@toto.the-meissners.org>
  

Ping for patches submitted on November 1st.  These 3 patches are needed to be
able to build GCC for PowerPC target systems where long double is configured to
be IEEE 128-bit, such as Fedora 36.

This is the first patch of 3 patches.

| Date: Tue, 1 Nov 2022 22:40:43 -0400
| Subject: [PATCH 1/3] Rework 128-bit complex multiply and divide, PR target/107299
| Message-ID: <Y2HYqx4zLCNCT0Zy@toto.the-meissners.org>
| https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604835.html
  

Hi Mike,

Thanks for fixing this!

on 2022/11/2 10:40, Michael Meissner wrote:
> This function reworks how the complex multiply and divide built-in functions are
> done.  Previously we created built-in declarations for doing long double complex
> multiply and divide when long double is IEEE 128-bit.  The old code also did not
> support __ibm128 complex multiply and divide if long double is IEEE 128-bit.
> 
> In terms of history, I wrote the original code just as I was starting to test
> GCC on systems where IEEE 128-bit long double was the default.  At the time, we
> had not yet started mangling the built-in function names as a way to bridge
> going from a system with 128-bit IBM long double to 128-bin IEEE long double.
                                                         ~~~ bit
> 
> The original code depends on there only being two 128-bit types invovled.  With
                                                                  ~~~~~~ involved.
> the next patch in this series, this assumption will no longer be true.  When
> long double is IEEE 128-bit, there will be 2 IEEE 128-bit types (one for the
> explicit __float128/_Float128 type and one for long double).
> 
> The problem is we cannot create two separate built-in functions that resolve to
> the same name.  This is a requirement of add_builtin_function and the C front
> end.  That means for the 3 possible modes (IFmode, KFmode, and TFmode), you can
> only use 2 of them.
> 
> This code does not create the built-in declaration with the changed name.
> Instead, it uses the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change the name
> before it is written out to the assembler file like it now does for all of the
> other long double built-in functions.
> 
> We need to disable using this mapping when we are building libgcc, specifically
> when it is building the floating point 128-bit multiply and divide functions.
> The flag that is used when libgcc is built (-fbuilding-libcc) is only available
> in the C/C++ front ends.  We need to remember that we are building libgcc in the
> rs6000-c.cc support to be able to use this later to decided whether to mangle
> the decl assembler name or not.

IIUC, for the building of floating point 128-bit multiply and divide functions,
the mapping seems still to work fine.  Using the multiply as example here, when
compiling _multc3.o, it's with -mabi=ibmlongdouble, it maps the name with "tc"
which is consistent with what we need.  While compiling _mulkc3*.o, we would
check the macro __LONG_DOUBLE_IEEE128__ and use either KCmode or TCmode, either
of the mapping result would be "kc".

Could you help to elaborate why we need to disable it during libgcc building?

BR,
Kewen

> 
> When I wrote these patches, I discovered that __ibm128 complex multiply and
> divide had originally not been supported if long double is IEEE 128-bit as it
> would generate calls to __mulic3 and __divic3.  I added tests in the testsuite
> to verify that the correct name (i.e. __multc3 and __divtc3) is used in this
> case.
> 
> I tested all 3 patchs for PR target/107299 on:
> 
>     1)	LE Power10 using --with-cpu=power10 --with-long-double-format=ieee
>     2)	LE Power10 using --with-cpu=power10 --with-long-double-format=ibm
>     3)	LE Power9  using --with-cpu=power9  --with-long-double-format=ibm
>     4)	BE Power8  using --with-cpu=power8  --with-long-double-format=ibm
> 
> Once all 3 patches have been applied, we can once again build GCC when long
> double is IEEE 128-bit.  There were no other regressions with these patches.
> Can I check these patches into the trunk?
> 
> 2022-11-01   Michael Meissner  <meissner@linux.ibm.com>
> 
> gcc/
> 
> 	PR target/107299
> 	* config/rs6000/rs6000-c.cc (rs6000_cpu_cpp_builtins): Set
> 	building_libgcc.
> 	* config/rs6000/rs6000.cc (create_complex_muldiv): Delete.
> 	(init_float128_ieee): Delete code to switch complex multiply and divide
> 	for long double.
> 	(complex_multiply_builtin_code): New helper function.
> 	(complex_divide_builtin_code): Likewise.
> 	(rs6000_mangle_decl_assembler_name): Add support for mangling the name
> 	of complex 128-bit multiply and divide built-in functions.
> 	* config/rs6000/rs6000.opt (building_libgcc): New target variable.
> 
> gcc/testsuite/
> 
> 	PR target/107299
> 	* gcc.target/powerpc/divic3-1.c: New test.
> 	* gcc.target/powerpc/divic3-2.c: Likewise.
> 	* gcc.target/powerpc/mulic3-1.c: Likewise.
> 	* gcc.target/powerpc/mulic3-2.c: Likewise.
> ---
>  gcc/config/rs6000/rs6000-c.cc               |   8 ++
>  gcc/config/rs6000/rs6000.cc                 | 110 +++++++++++---------
>  gcc/config/rs6000/rs6000.opt                |   4 +
>  gcc/testsuite/gcc.target/powerpc/divic3-1.c |  18 ++++
>  gcc/testsuite/gcc.target/powerpc/divic3-2.c |  17 +++
>  gcc/testsuite/gcc.target/powerpc/mulic3-1.c |  18 ++++
>  gcc/testsuite/gcc.target/powerpc/mulic3-2.c |  17 +++
>  7 files changed, 145 insertions(+), 47 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/divic3-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/divic3-2.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/mulic3-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/mulic3-2.c
> 
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index 56609462629..5c2f3bcee9f 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -780,6 +780,14 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfile)
>        || DEFAULT_ABI == ABI_ELFv2
>        || (DEFAULT_ABI == ABI_AIX && !rs6000_compat_align_parm))
>      builtin_define ("__STRUCT_PARM_ALIGN__=16");
> +
> +  /* Store whether or not we are building libgcc.  This is needed to disable
> +     generating the alternate names for 128-bit complex multiply and divide.
> +     We need to disable generating __multc3, __divtc3, __mulkc3, and __divkc3
> +     when we are building those functions in libgcc.  The variable
> +     flag_building_libgcc is only available for the C family of front-ends.
> +     We set this variable here to disable generating the alternate names.  */
> +  building_libgcc = flag_building_libgcc;
>  }
>  
>  
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index a85d7630b41..cfb6227e27b 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -11123,26 +11123,6 @@ init_float128_ibm (machine_mode mode)
>      }
>  }
>  
> -/* Create a decl for either complex long double multiply or complex long double
> -   divide when long double is IEEE 128-bit floating point.  We can't use
> -   __multc3 and __divtc3 because the original long double using IBM extended
> -   double used those names.  The complex multiply/divide functions are encoded
> -   as builtin functions with a complex result and 4 scalar inputs.  */
> -
> -static void
> -create_complex_muldiv (const char *name, built_in_function fncode, tree fntype)
> -{
> -  tree fndecl = add_builtin_function (name, fntype, fncode, BUILT_IN_NORMAL,
> -				      name, NULL_TREE);
> -
> -  set_builtin_decl (fncode, fndecl, true);
> -
> -  if (TARGET_DEBUG_BUILTIN)
> -    fprintf (stderr, "create complex %s, fncode: %d\n", name, (int) fncode);
> -
> -  return;
> -}
> -
>  /* Set up IEEE 128-bit floating point routines.  Use different names if the
>     arguments can be passed in a vector register.  The historical PowerPC
>     implementation of IEEE 128-bit floating point used _q_<op> for the names, so
> @@ -11154,32 +11134,6 @@ init_float128_ieee (machine_mode mode)
>  {
>    if (FLOAT128_VECTOR_P (mode))
>      {
> -      static bool complex_muldiv_init_p = false;
> -
> -      /* Set up to call __mulkc3 and __divkc3 under -mabi=ieeelongdouble.  If
> -	 we have clone or target attributes, this will be called a second
> -	 time.  We want to create the built-in function only once.  */
> -     if (mode == TFmode && TARGET_IEEEQUAD && !complex_muldiv_init_p)
> -       {
> -	 complex_muldiv_init_p = true;
> -	 built_in_function fncode_mul =
> -	   (built_in_function) (BUILT_IN_COMPLEX_MUL_MIN + TCmode
> -				- MIN_MODE_COMPLEX_FLOAT);
> -	 built_in_function fncode_div =
> -	   (built_in_function) (BUILT_IN_COMPLEX_DIV_MIN + TCmode
> -				- MIN_MODE_COMPLEX_FLOAT);
> -
> -	 tree fntype = build_function_type_list (complex_long_double_type_node,
> -						 long_double_type_node,
> -						 long_double_type_node,
> -						 long_double_type_node,
> -						 long_double_type_node,
> -						 NULL_TREE);
> -
> -	 create_complex_muldiv ("__mulkc3", fncode_mul, fntype);
> -	 create_complex_muldiv ("__divkc3", fncode_div, fntype);
> -       }
> -
>        set_optab_libfunc (add_optab, mode, "__addkf3");
>        set_optab_libfunc (sub_optab, mode, "__subkf3");
>        set_optab_libfunc (neg_optab, mode, "__negkf2");
> @@ -28142,6 +28096,25 @@ rs6000_starting_frame_offset (void)
>    return RS6000_STARTING_FRAME_OFFSET;
>  }
>  
> +/* Internal function to return the built-in function id for the complex
> +   multiply operation for a given mode.  */
> +
> +static inline built_in_function
> +complex_multiply_builtin_code (machine_mode mode)
> +{
> +  return (built_in_function) (BUILT_IN_COMPLEX_MUL_MIN + mode
> +			      - MIN_MODE_COMPLEX_FLOAT);
> +}
> +
> +/* Internal function to return the built-in function id for the complex divide
> +   operation for a given mode.  */
> +
> +static inline built_in_function
> +complex_divide_builtin_code (machine_mode mode)
> +{
> +  return (built_in_function) (BUILT_IN_COMPLEX_DIV_MIN + mode
> +			      - MIN_MODE_COMPLEX_FLOAT);
> +}
>  
>  /* On 64-bit Linux and Freebsd systems, possibly switch the long double library
>     function names from <foo>l to <foo>f128 if the default long double type is
> @@ -28160,11 +28133,54 @@ rs6000_starting_frame_offset (void)
>     only do this transformation if the __float128 type is enabled.  This
>     prevents us from doing the transformation on older 32-bit ports that might
>     have enabled using IEEE 128-bit floating point as the default long double
> -   type.  */
> +   type.
> +
> +   We also use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change the
> +   function names used for complex multiply and divide to the appropriate
> +   names.  */
>  
>  static tree
>  rs6000_mangle_decl_assembler_name (tree decl, tree id)
>  {
> +  /* Handle complex multiply/divide.  For IEEE 128-bit, use __mulkc3 or
> +     __divkc3 and for IBM 128-bit use __multc3 and __divtc3.  */
> +  if (TARGET_FLOAT128_TYPE
> +      && !building_libgcc
> +      && TREE_CODE (decl) == FUNCTION_DECL
> +      && DECL_IS_UNDECLARED_BUILTIN (decl)
> +      && DECL_BUILT_IN_CLASS (decl) == BUILT_IN_NORMAL)
> +    {
> +      built_in_function id = DECL_FUNCTION_CODE (decl);
> +      const char *newname = NULL;
> +
> +      if (id == complex_multiply_builtin_code (KCmode))
> +	newname = "__mulkc3";
> +
> +      else if (id == complex_multiply_builtin_code (ICmode))
> +	newname = "__multc3";
> +
> +      else if (id == complex_multiply_builtin_code (TCmode))
> +	newname = (TARGET_IEEEQUAD) ? "__mulkc3" : "__multc3";
> +
> +      else if (id == complex_divide_builtin_code (KCmode))
> +	newname = "__divkc3";
> +
> +      else if (id == complex_divide_builtin_code (ICmode))
> +	newname = "__divtc3";
> +
> +      else if (id == complex_divide_builtin_code (TCmode))
> +	newname = (TARGET_IEEEQUAD) ? "__divkc3" : "__divtc3";
> +
> +      if (newname)
> +	{
> +	  if (TARGET_DEBUG_BUILTIN)
> +	    fprintf (stderr, "Map complex mul/div => %s\n", newname);
> +
> +	  return get_identifier (newname);
> +	}
> +    }
> +
> +  /* Map long double built-in functions if long double is IEEE 128-bit.  */
>    if (TARGET_FLOAT128_TYPE && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128
>        && TREE_CODE (decl) == FUNCTION_DECL
>        && DECL_IS_UNDECLARED_BUILTIN (decl)
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index b63a5d443af..e2de03dda92 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -100,6 +100,10 @@ unsigned int rs6000_recip_control
>  TargetVariable
>  unsigned int rs6000_debug
>  
> +;; Whether we are building libgcc or not.
> +TargetVariable
> +bool building_libgcc = false
> +
>  ;; Whether to enable the -mfloat128 stuff without necessarily enabling the
>  ;; __float128 keyword.
>  TargetSave
> diff --git a/gcc/testsuite/gcc.target/powerpc/divic3-1.c b/gcc/testsuite/gcc.target/powerpc/divic3-1.c
> new file mode 100644
> index 00000000000..1cc6b1be904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/divic3-1.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-require-effective-target longdouble128 } */
> +/* { dg-require-effective-target ppc_float128_sw } */
> +/* { dg-options "-O2 -mpower8-vector -mabi=ieeelongdouble -Wno-psabi" } */
> +
> +/* Check that complex divide generates the right call for __ibm128 when long
> +   double is IEEE 128-bit floating point.  */
> +
> +typedef _Complex long double c_ibm128_t __attribute__((mode(__IC__)));
> +
> +void
> +divide (c_ibm128_t *p, c_ibm128_t *q, c_ibm128_t *r)
> +{
> +  *p = *q / *r;
> +}
> +
> +/* { dg-final { scan-assembler "bl __divtc3" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/divic3-2.c b/gcc/testsuite/gcc.target/powerpc/divic3-2.c
> new file mode 100644
> index 00000000000..8ff342e0116
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/divic3-2.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-require-effective-target longdouble128 } */
> +/* { dg-options "-O2 -mpower8-vector -mabi=ibmlongdouble -Wno-psabi" } */
> +
> +/* Check that complex divide generates the right call for __ibm128 when long
> +   double is IBM 128-bit floating point.  */
> +
> +typedef _Complex long double c_ibm128_t __attribute__((mode(__TC__)));
> +
> +void
> +divide (c_ibm128_t *p, c_ibm128_t *q, c_ibm128_t *r)
> +{
> +  *p = *q / *r;
> +}
> +
> +/* { dg-final { scan-assembler "bl __divtc3" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/mulic3-1.c b/gcc/testsuite/gcc.target/powerpc/mulic3-1.c
> new file mode 100644
> index 00000000000..4cd773c4b06
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/mulic3-1.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-require-effective-target longdouble128 } */
> +/* { dg-require-effective-target ppc_float128_sw } */
> +/* { dg-options "-O2 -mpower8-vector -mabi=ieeelongdouble -Wno-psabi" } */
> +
> +/* Check that complex multiply generates the right call for __ibm128 when long
> +   double is IEEE 128-bit floating point.  */
> +
> +typedef _Complex long double c_ibm128_t __attribute__((mode(__IC__)));
> +
> +void
> +multiply (c_ibm128_t *p, c_ibm128_t *q, c_ibm128_t *r)
> +{
> +  *p = *q * *r;
> +}
> +
> +/* { dg-final { scan-assembler "bl __multc3" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/mulic3-2.c b/gcc/testsuite/gcc.target/powerpc/mulic3-2.c
> new file mode 100644
> index 00000000000..36fe8bc3061
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/mulic3-2.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-require-effective-target longdouble128 } */
> +/* { dg-options "-O2 -mpower8-vector -mabi=ibmlongdouble -Wno-psabi" } */
> +
> +/* Check that complex multiply generates the right call for __ibm128 when long
> +   double is IBM 128-bit floating point.  */
> +
> +typedef _Complex long double c_ibm128_t __attribute__((mode(__TC__)));
> +
> +void
> +multiply (c_ibm128_t *p, c_ibm128_t *q, c_ibm128_t *r)
> +{
> +  *p = *q * *r;
> +}
> +
> +/* { dg-final { scan-assembler "bl __multc3" } } */
  

On Tue, Dec 06, 2022 at 05:36:54PM +0800, Kewen.Lin wrote:
> Hi Mike,
> 
> Thanks for fixing this!
> 
> Could you help to elaborate why we need to disable it during libgcc building?

When you are building libgcc, you are building the __mulkc3, __divkc3
functions.  The mapping in the compiler interferes with those functions,
because at the moment, libgcc uses an alternate IEEE 128-bit type.

I have a patch for making libgcc use the 'right' type that I haven't submitted
yet.  This is because the more general fix that these 3 patches do impacts other
functions (due to __float128 and _Float128 being different in the current
compiler when -mabi=ieeelongdouble).
  

Hi Mike,

on 2022/12/7 14:44, Michael Meissner wrote:
> On Tue, Dec 06, 2022 at 05:36:54PM +0800, Kewen.Lin wrote:
>> Hi Mike,
>>
>> Thanks for fixing this!
>>
>> Could you help to elaborate why we need to disable it during libgcc building?
> 
> When you are building libgcc, you are building the __mulkc3, __divkc3
> functions.  The mapping in the compiler interferes with those functions,
> because at the moment, libgcc uses an alternate IEEE 128-bit type.
> 

But I'm still confused.  For __mulkc3 (__divkc3 is similar),

1) with -mabi=ieeelongdouble (TARGET_IEEEQUAD true, define __LONG_DOUBLE_IEEE128__),
   the used types are:

   typedef float TFtype __attribute__ ((mode (TF)));
   typedef __complex float TCtype __attribute__ ((mode (TC)));

2) with -mabi=ibmlongdouble (TARGET_IEEEQUAD false, not __LONG_DOUBLE_IEEE128__ defined),
   the used types are:

   typedef float TFtype __attribute__ ((mode (KF)));
   typedef __complex float TCtype __attribute__ ((mode (KC)));

The proposed mapping in the current patch is:

+
+      if (id == complex_multiply_builtin_code (KCmode))
+	newname = "__mulkc3";
+
+      else if (id == complex_multiply_builtin_code (ICmode))
+	newname = "__multc3";
+
+      else if (id == complex_multiply_builtin_code (TCmode))
+	newname = (TARGET_IEEEQUAD) ? "__mulkc3" : "__multc3";

for 1), TCmode && TARGET_IEEEQUAD => "__mulkc3"
for 2), KCmode => "__mulkc3"

Both should be still with name "__mulkc3", do I miss anything?

BR,
Kewen

> I have a patch for making libgcc use the 'right' type that I haven't submitted
> yet.  This is because the more general fix that these 3 patches do impacts other
> functions (due to __float128 and _Float128 being different in the current
> compiler when -mabi=ieeelongdouble).
>
  

On Wed, Dec 07, 2022 at 03:55:41PM +0800, Kewen.Lin wrote:
> Hi Mike,
> 
> on 2022/12/7 14:44, Michael Meissner wrote:
> > On Tue, Dec 06, 2022 at 05:36:54PM +0800, Kewen.Lin wrote:
> >> Hi Mike,
> >>
> >> Thanks for fixing this!
> >>
> >> Could you help to elaborate why we need to disable it during libgcc building?
> > 
> > When you are building libgcc, you are building the __mulkc3, __divkc3
> > functions.  The mapping in the compiler interferes with those functions,
> > because at the moment, libgcc uses an alternate IEEE 128-bit type.
> > 
> 
> But I'm still confused.  For __mulkc3 (__divkc3 is similar),
> 
> 1) with -mabi=ieeelongdouble (TARGET_IEEEQUAD true, define __LONG_DOUBLE_IEEE128__),
>    the used types are:
> 
>    typedef float TFtype __attribute__ ((mode (TF)));
>    typedef __complex float TCtype __attribute__ ((mode (TC)));
> 
> 2) with -mabi=ibmlongdouble (TARGET_IEEEQUAD false, not __LONG_DOUBLE_IEEE128__ defined),
>    the used types are:
> 
>    typedef float TFtype __attribute__ ((mode (KF)));
>    typedef __complex float TCtype __attribute__ ((mode (KC)));
> 
> The proposed mapping in the current patch is:
> 
> +
> +      if (id == complex_multiply_builtin_code (KCmode))
> +	newname = "__mulkc3";
> +
> +      else if (id == complex_multiply_builtin_code (ICmode))
> +	newname = "__multc3";
> +
> +      else if (id == complex_multiply_builtin_code (TCmode))
> +	newname = (TARGET_IEEEQUAD) ? "__mulkc3" : "__multc3";
> 
> for 1), TCmode && TARGET_IEEEQUAD => "__mulkc3"
> for 2), KCmode => "__mulkc3"
> 
> Both should be still with name "__mulkc3", do I miss anything?
> 
> BR,
> Kewen

The reason is due to the different internal types, the value range propigation
pass throws an error when we are trying to build libgcc.  This is due to the
underlying problem of different IEEE 128-bit types within the compiler.

The 128-bit IEEE support in libgcc was written before _Float128 was added to
GCC.  One consequence is that you can't get to the complex variant of
__float128.  So libgcc needs to use the attribute mode to get to that type.

But with the support for IEEE 128-bit long double changing things, it makes the
libgcc code use the wrong code.

/home/meissner/fsf-src/work102/libgcc/config/rs6000/_mulkc3.c: In function ‘__mulkc3_sw’:
/home/meissner/fsf-src/work102/libgcc/config/rs6000/_mulkc3.c:97:1: internal compiler error: in fold_stmt, at gimple-range-fold.cc:522
   97 | }
      | ^
0x122784f3 fold_using_range::fold_stmt(vrange&, gimple*, fur_source&, tree_node*)
        /home/meissner/fsf-src/work102/gcc/gimple-range-fold.cc:522
0x1226477f gimple_ranger::fold_range_internal(vrange&, gimple*, tree_node*)
        /home/meissner/fsf-src/work102/gcc/gimple-range.cc:257
0x12264b1f gimple_ranger::range_of_stmt(vrange&, gimple*, tree_node*)
        /home/meissner/fsf-src/work102/gcc/gimple-range.cc:318
0x113bdd8b range_query::value_of_stmt(gimple*, tree_node*)
        /home/meissner/fsf-src/work102/gcc/value-query.cc:134
0x1134838f rvrp_folder::value_of_stmt(gimple*, tree_node*)
        /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1023
0x111344cf substitute_and_fold_dom_walker::before_dom_children(basic_block_def*)
        /home/meissner/fsf-src/work102/gcc/tree-ssa-propagate.cc:819
0x121ecbd3 dom_walker::walk(basic_block_def*)
        /home/meissner/fsf-src/work102/gcc/domwalk.cc:311
0x11134ee7 substitute_and_fold_engine::substitute_and_fold(basic_block_def*)
        /home/meissner/fsf-src/work102/gcc/tree-ssa-propagate.cc:998
0x11346bb7 execute_ranger_vrp(function*, bool, bool)
        /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1084
0x11347063 execute
        /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1165
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
make[1]: *** [/home/meissner/fsf-src/work102/libgcc/shared-object.mk:14: _mulkc3.o] Error 1
make[1]: Leaving directory '/home/meissner/fsf-build-ppc64le/work102/powerpc64le-unknown-linux-gnu/libgcc'
make: *** [Makefile:20623: all-target-libgcc] Error 2

> > I have a patch for making libgcc use the 'right' type that I haven't submitted
> > yet.  This is because the more general fix that these 3 patches do impacts other
> > functions (due to __float128 and _Float128 being different in the current
> > compiler when -mabi=ieeelongdouble).
> > 

The patch is to use _Float128 and _Complex _Float128 in libgcc.h instead of
trying to use attribute((mode(TF))) and attribute((mode(TC))) in libgcc.

Now, this patch fixes the specific problem of not being able to build libgcc
(along with patch #1 of the series).  But other things show the differences
from time time because we are using different internal types and the middle end
doesn't know that these types are really the same bits.

It is better long term (IMHO) if we have the two types (__float128 and
_Float128) use the same internal type (which is what is done in patches #2 and
#3).  This fixes the other issues that show up, such as creating signaling NaNs
for one internal type, and converting it to the other internal type, loses that
the NaN is signalling.
  

on 2022/12/9 06:04, Michael Meissner wrote:
> On Wed, Dec 07, 2022 at 03:55:41PM +0800, Kewen.Lin wrote:
>> Hi Mike,
>>
>> on 2022/12/7 14:44, Michael Meissner wrote:
>>> On Tue, Dec 06, 2022 at 05:36:54PM +0800, Kewen.Lin wrote:
>>>> Hi Mike,
>>>>
>>>> Thanks for fixing this!
>>>>
>>>> Could you help to elaborate why we need to disable it during libgcc building?
>>>
>>> When you are building libgcc, you are building the __mulkc3, __divkc3
>>> functions.  The mapping in the compiler interferes with those functions,
>>> because at the moment, libgcc uses an alternate IEEE 128-bit type.
>>>
>>
>> But I'm still confused.  For __mulkc3 (__divkc3 is similar),
>>
>> 1) with -mabi=ieeelongdouble (TARGET_IEEEQUAD true, define __LONG_DOUBLE_IEEE128__),
>>    the used types are:
>>
>>    typedef float TFtype __attribute__ ((mode (TF)));
>>    typedef __complex float TCtype __attribute__ ((mode (TC)));
>>
>> 2) with -mabi=ibmlongdouble (TARGET_IEEEQUAD false, not __LONG_DOUBLE_IEEE128__ defined),
>>    the used types are:
>>
>>    typedef float TFtype __attribute__ ((mode (KF)));
>>    typedef __complex float TCtype __attribute__ ((mode (KC)));
>>
>> The proposed mapping in the current patch is:
>>
>> +
>> +      if (id == complex_multiply_builtin_code (KCmode))
>> +	newname = "__mulkc3";
>> +
>> +      else if (id == complex_multiply_builtin_code (ICmode))
>> +	newname = "__multc3";
>> +
>> +      else if (id == complex_multiply_builtin_code (TCmode))
>> +	newname = (TARGET_IEEEQUAD) ? "__mulkc3" : "__multc3";
>>
>> for 1), TCmode && TARGET_IEEEQUAD => "__mulkc3"
>> for 2), KCmode => "__mulkc3"
>>
>> Both should be still with name "__mulkc3", do I miss anything?
>>
>> BR,
>> Kewen
> 
> The reason is due to the different internal types, the value range propigation
> pass throws an error when we are trying to build libgcc.  This is due to the
> underlying problem of different IEEE 128-bit types within the compiler.
> 

But this is the reason why we need patch #2 and #3, not the reason why we need
the special handling for building_libgcc in patch #1, right?

Without or with patch #1, the below ICE in libgcc exists, the ICE should have
nothing to do with the special handling for building_libgcc in patch #1.  I
think patch #2 which makes _Float128 and __float128 use the same internal
type fixes that ICE.

I still don't get the point why we need the special handling for building_libgcc,
I also tested on top of patch #1 and #2 w/ and w/o the special handling for
building_libgcc, both bootstrapped and regress-tested.

Could you have a double check?

> The 128-bit IEEE support in libgcc was written before _Float128 was added to
> GCC.  One consequence is that you can't get to the complex variant of
> __float128.  So libgcc needs to use the attribute mode to get to that type.
> 
> But with the support for IEEE 128-bit long double changing things, it makes the
> libgcc code use the wrong code.
> 
> /home/meissner/fsf-src/work102/libgcc/config/rs6000/_mulkc3.c: In function ‘__mulkc3_sw’:
> /home/meissner/fsf-src/work102/libgcc/config/rs6000/_mulkc3.c:97:1: internal compiler error: in fold_stmt, at gimple-range-fold.cc:522
>    97 | }
>       | ^
> 0x122784f3 fold_using_range::fold_stmt(vrange&, gimple*, fur_source&, tree_node*)
>         /home/meissner/fsf-src/work102/gcc/gimple-range-fold.cc:522
> 0x1226477f gimple_ranger::fold_range_internal(vrange&, gimple*, tree_node*)
>         /home/meissner/fsf-src/work102/gcc/gimple-range.cc:257
> 0x12264b1f gimple_ranger::range_of_stmt(vrange&, gimple*, tree_node*)
>         /home/meissner/fsf-src/work102/gcc/gimple-range.cc:318
> 0x113bdd8b range_query::value_of_stmt(gimple*, tree_node*)
>         /home/meissner/fsf-src/work102/gcc/value-query.cc:134
> 0x1134838f rvrp_folder::value_of_stmt(gimple*, tree_node*)
>         /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1023
> 0x111344cf substitute_and_fold_dom_walker::before_dom_children(basic_block_def*)
>         /home/meissner/fsf-src/work102/gcc/tree-ssa-propagate.cc:819
> 0x121ecbd3 dom_walker::walk(basic_block_def*)
>         /home/meissner/fsf-src/work102/gcc/domwalk.cc:311
> 0x11134ee7 substitute_and_fold_engine::substitute_and_fold(basic_block_def*)
>         /home/meissner/fsf-src/work102/gcc/tree-ssa-propagate.cc:998
> 0x11346bb7 execute_ranger_vrp(function*, bool, bool)
>         /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1084
> 0x11347063 execute
>         /home/meissner/fsf-src/work102/gcc/tree-vrp.cc:1165
> Please submit a full bug report, with preprocessed source (by using -freport-bug).
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
> make[1]: *** [/home/meissner/fsf-src/work102/libgcc/shared-object.mk:14: _mulkc3.o] Error 1
> make[1]: Leaving directory '/home/meissner/fsf-build-ppc64le/work102/powerpc64le-unknown-linux-gnu/libgcc'
> make: *** [Makefile:20623: all-target-libgcc] Error 2
> 
>>> I have a patch for making libgcc use the 'right' type that I haven't submitted
>>> yet.  This is because the more general fix that these 3 patches do impacts other
>>> functions (due to __float128 and _Float128 being different in the current
>>> compiler when -mabi=ieeelongdouble).
>>>
> 
> The patch is to use _Float128 and _Complex _Float128 in libgcc.h instead of
> trying to use attribute((mode(TF))) and attribute((mode(TC))) in libgcc.
> 

Since your patch #2 (and #3) fixes ICE and some exposed problems, and _Float128
is to use the same internal type as __float128, types with attribute((mode(TF)))
and attribute((mode(TC))) should be correct, I assume that this patch is just
to make the types explicit be with _Float128 (for better readability and
maintainance), but not for any correctness issues.

> Now, this patch fixes the specific problem of not being able to build libgcc
> (along with patch #1 of the series).  But other things show the differences
> from time time because we are using different internal types and the middle end
> doesn't know that these types are really the same bits.
> 
> It is better long term (IMHO) if we have the two types (__float128 and
> _Float128) use the same internal type (which is what is done in patches #2 and
> #3).  This fixes the other issues that show up, such as creating signaling NaNs
> for one internal type, and converting it to the other internal type, loses that
> the NaN is signalling.
> 

I see, nice!

BR,
Kewen
  

On Mon, Dec 12, 2022 at 06:20:14PM +0800, Kewen.Lin wrote:
> Without or with patch #1, the below ICE in libgcc exists, the ICE should have
> nothing to do with the special handling for building_libgcc in patch #1.  I
> think patch #2 which makes _Float128 and __float128 use the same internal
> type fixes that ICE.
> 
> I still don't get the point why we need the special handling for building_libgcc,
> I also tested on top of patch #1 and #2 w/ and w/o the special handling for
> building_libgcc, both bootstrapped and regress-tested.
> 
> Could you have a double check?

As long as patch #2 and #3 are installed, we don't need the special handling
for building_libgcc.  Good catch.

I will send out a replacement patch for it.

> Since your patch #2 (and #3) fixes ICE and some exposed problems, and _Float128
> is to use the same internal type as __float128, types with attribute((mode(TF)))
> and attribute((mode(TC))) should be correct, I assume that this patch is just
> to make the types explicit be with _Float128 (for better readability and
> maintainance), but not for any correctness issues.

Yes, the patch is mainly for clarity.  The history is the libgcc support went
in before _Float128 went in, and I never went back to use those types when we
could use them.

With _Float128, we can just use _Complex _Float128 and not
bother with trying to get the right KC/TC for the attribute mode stuff.

However, if patches 1-3 aren't put in, just applying the patch to use _Float128
and _Complex _Float128 would fix the immediate problem (of not building GCC on
systems with IEEE 128-bit long double).  However, it is a band-aid that just
works around the problem of building __mulkc3 and __divkc3.  It doesn't fix the
other problems between __float128 and _Float128 that show up in some places
that I would like to get fixed.

So I haven't submitted the patch, because I think it is more important to get
the other issues fixed.

> > Now, this patch fixes the specific problem of not being able to build libgcc
> > (along with patch #1 of the series).  But other things show the differences
> > from time time because we are using different internal types and the middle end
> > doesn't know that these types are really the same bits.
> > 
> > It is better long term (IMHO) if we have the two types (__float128 and
> > _Float128) use the same internal type (which is what is done in patches #2 and
> > #3).  This fixes the other issues that show up, such as creating signaling NaNs
> > for one internal type, and converting it to the other internal type, loses that
> > the NaN is signalling.
> > 
> 
> I see, nice!
> 
> BR,
> Kewen
  

I have submitted a new replacement patch for this patch:

| Date: Tue, 1 Nov 2022 22:40:43 -0400
| Subject: [PATCH 1/3] Rework 128-bit complex multiply and divide, PR target/107299
| Message-ID: <Y2HYqx4zLCNCT0Zy@toto.the-meissners.org>
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608368.html
  

On Tue, Dec 13, 2022 at 01:14:39AM -0500, Michael Meissner wrote:
> On Mon, Dec 12, 2022 at 06:20:14PM +0800, Kewen.Lin wrote:
> > Without or with patch #1, the below ICE in libgcc exists, the ICE should have
> > nothing to do with the special handling for building_libgcc in patch #1.  I
> > think patch #2 which makes _Float128 and __float128 use the same internal
> > type fixes that ICE.
> > 
> > I still don't get the point why we need the special handling for building_libgcc,
> > I also tested on top of patch #1 and #2 w/ and w/o the special handling for
> > building_libgcc, both bootstrapped and regress-tested.
> > 
> > Could you have a double check?
> 
> As long as patch #2 and #3 are installed, we don't need the special handling
> for building_libgcc.  Good catch.
> 
> I will send out a replacement patch for it.

Please send a complete new series replacing this one.  Thanks.


Segher
  

on 2022/12/13 14:14, Michael Meissner wrote:
> On Mon, Dec 12, 2022 at 06:20:14PM +0800, Kewen.Lin wrote:
>> Without or with patch #1, the below ICE in libgcc exists, the ICE should have
>> nothing to do with the special handling for building_libgcc in patch #1.  I
>> think patch #2 which makes _Float128 and __float128 use the same internal
>> type fixes that ICE.
>>
>> I still don't get the point why we need the special handling for building_libgcc,
>> I also tested on top of patch #1 and #2 w/ and w/o the special handling for
>> building_libgcc, both bootstrapped and regress-tested.
>>
>> Could you have a double check?
> 
> As long as patch #2 and #3 are installed, we don't need the special handling
> for building_libgcc.  Good catch.
> 
> I will send out a replacement patch for it.

Thanks!  I still feel patch #1 is independent, it helps to fix the issues as
shown in its associated test case, which looks an oversight in the previous
implementation to me. :)

> 
>> Since your patch #2 (and #3) fixes ICE and some exposed problems, and _Float128
>> is to use the same internal type as __float128, types with attribute((mode(TF)))
>> and attribute((mode(TC))) should be correct, I assume that this patch is just
>> to make the types explicit be with _Float128 (for better readability and
>> maintainance), but not for any correctness issues.
> 
> Yes, the patch is mainly for clarity.  The history is the libgcc support went
> in before _Float128 went in, and I never went back to use those types when we
> could use them.
> 
> With _Float128, we can just use _Complex _Float128 and not
> bother with trying to get the right KC/TC for the attribute mode stuff.
> 
> However, if patches 1-3 aren't put in, just applying the patch to use _Float128
> and _Complex _Float128 would fix the immediate problem (of not building GCC on
> systems with IEEE 128-bit long double).  However, it is a band-aid that just
> works around the problem of building __mulkc3 and __divkc3.  It doesn't fix the
> other problems between __float128 and _Float128 that show up in some places
> that I would like to get fixed.
> 
> So I haven't submitted the patch, because I think it is more important to get
> the other issues fixed.

OK, make sense, thanks for the clarification!

BR,
Kewen