Re: [PATCH v2, rs6000] Add multiply-add expand pattern [PR103109]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Haochen,

Thanks for the patch.

on 2022/8/8 14:04, HAO CHEN GUI wrote:
> Hi,
>   This patch adds an expand and several insns for multiply-add with three
> 64bit operands.
> 
>   Compared with last version, the main changes are:
> 1 The "maddld" pattern is reused for the low-part generation.
> 2 A runnable testcase replaces the original compiling case.
> 3 Fixes indention problems.
> 
>   Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> ChangeLog
> 2022-08-08  Haochen Gui  <guihaoc@linux.ibm.com>
> 
> gcc/
> 	PR target/103109
> 	* config/rs6000/rs6000.md (<u>maddditi4): New pattern for multiply-add.
> 	(<u>madddi4_highpart): New.
> 	(<u>madddi4_highpart_le): New.
> 
> gcc/testsuite/
> 	PR target/103109
> 	* gcc.target/powerpc/pr103109.c: New.
> 
> 
> 
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index c55ee7e171a..4c58023490a 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -3217,7 +3217,7 @@ (define_expand "<u>mul<mode><dmode>3"
>    DONE;
>  })
> 
> -(define_insn "*maddld<mode>4"
> +(define_insn "maddld<mode>4"
>    [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
>  	(plus:GPR (mult:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
>  			    (match_operand:GPR 2 "gpc_reg_operand" "r"))
> @@ -3226,6 +3226,52 @@ (define_insn "*maddld<mode>4"
>    "maddld %0,%1,%2,%3"
>    [(set_attr "type" "mul")])
> 
> +(define_expand "<u>maddditi4"
> +  [(set (match_operand:TI 0 "gpc_reg_operand")
> +	(plus:TI
> +	  (mult:TI (any_extend:TI (match_operand:DI 1 "gpc_reg_operand"))
> +		   (any_extend:TI (match_operand:DI 2 "gpc_reg_operand")))
> +	  (any_extend:TI (match_operand:DI 3 "gpc_reg_operand"))))]
> +  "TARGET_MADDLD && TARGET_POWERPC64"
> +{
> +  rtx op0_lo = gen_rtx_SUBREG (DImode, operands[0], BYTES_BIG_ENDIAN ? 8 : 0);
> +  rtx op0_hi = gen_rtx_SUBREG (DImode, operands[0], BYTES_BIG_ENDIAN ? 0 : 8);
> +
> +  emit_insn (gen_maddlddi4 (op0_lo, operands[1], operands[2], operands[3]));
> +
> +  if (BYTES_BIG_ENDIAN)
> +    emit_insn (gen_<u>madddi4_highpart (op0_hi, operands[1], operands[2],
> +					operands[3]));
> +  else
> +    emit_insn (gen_<u>madddi4_highpart_le (op0_hi, operands[1], operands[2],
> +					   operands[3]));
> +  DONE;
> +})
> +
> +(define_insn "<u>madddi4_highpart"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> +	(subreg:DI
> +	  (plus:TI
> +	    (mult:TI (any_extend:TI (match_operand:DI 1 "gpc_reg_operand" "r"))
> +		     (any_extend:TI (match_operand:DI 2 "gpc_reg_operand" "r")))
> +	    (any_extend:TI (match_operand:DI 3 "gpc_reg_operand" "r")))
> +	 0))]
> +  "TARGET_MADDLD && BYTES_BIG_ENDIAN && TARGET_POWERPC64"
> +  "maddhd<u> %0,%1,%2,%3"
> +  [(set_attr "type" "mul")])
> +
> +(define_insn "<u>madddi4_highpart_le"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> +	(subreg:DI
> +	  (plus:TI
> +	    (mult:TI (any_extend:TI (match_operand:DI 1 "gpc_reg_operand" "r"))
> +		     (any_extend:TI (match_operand:DI 2 "gpc_reg_operand" "r")))
> +	    (any_extend:TI (match_operand:DI 3 "gpc_reg_operand" "r")))
> +	 8))]
> +  "TARGET_MADDLD && !BYTES_BIG_ENDIAN && TARGET_POWERPC64"
> +  "maddhd<u> %0,%1,%2,%3"
> +  [(set_attr "type" "mul")])
> +
>  (define_insn "udiv<mode>3"
>    [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
>          (udiv:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103109.c b/gcc/testsuite/gcc.target/powerpc/pr103109.c
> new file mode 100644
> index 00000000000..969b9751b21
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr103109.c
> @@ -0,0 +1,110 @@
> +/* { dg-do run { target { has_arch_ppc64 } } } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power9 -save-temps" } */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-require-effective-target p9modulo_hw } */
> +/* { dg-final { scan-assembler-times {\mmaddld\M} 2 } } */
> +/* { dg-final { scan-assembler-times {\mmaddhd\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mmaddhdu\M} 1 } } */
> +

Maybe it's good to split this case into two, one for compiling and the other for running.
Since the generated asm is a test point here, with one separated case for compiling, we
can still have that part of test coverage on hosts which are unable to run this case.
You can move functions multiply_add and multiply_addu into one common header file, then
include it in both source files.

> +union U {
> +  __int128 i128;
> +  struct {
> +    long l1;
> +    long l2;
> +  } s;
> +};
> +
> +__int128
> +create_i128 (long most_sig, long least_sig)
> +{
> +  union U u;
> +
> +#if __LITTLE_ENDIAN__
> +  u.s.l1 = least_sig;
> +  u.s.l2 = most_sig;
> +#else
> +  u.s.l1 = most_sig;
> +  u.s.l2 = least_sig;
> +#endif
> +  return u.i128;
> +}
> +
> +
> +#define DEBUG 0
> +
> +#if DEBUG
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +void print_i128(__int128 val, int unsignedp)
> +{
> +  if (unsignedp)
> +    printf(" %llu ", (unsigned long long)(val >> 64));
> +  else
> +    printf(" %lld ", (signed long long)(val >> 64));
> +
> +  printf("%llu (0x%llx %llx)",
> +         (unsigned long long)(val & 0xFFFFFFFFFFFFFFFF),
> +         (unsigned long long)(val >> 64),
> +         (unsigned long long)(val & 0xFFFFFFFFFFFFFFFF));
> +}
> +#endif
> +
> +void abort (void);
> +
> +__attribute__((noinline))
> +__int128 multiply_add (long a, long b, long c)
> +{
> +  return (__int128) a * b + c;
> +}
> +
> +__attribute__((noinline))
> +unsigned __int128 multiply_addu (unsigned long a, unsigned long b,
> +				 unsigned long c)
> +{
> +  return (unsigned __int128) a * b + c;
> +}
> +
> +int main ()
> +{
> +  long a = 0xFEDCBA9876543210L;
> +  long b = 0x1000000L;
> +  long c = 0x123456L;
> +  __int128 expected_result = create_i128 (0xFFFFFFFFFFFEDCBAL,
> +					  0x9876543210123456L);
> +
> +  __int128 result = multiply_add (a, b, c);
> +
> +  if (result != expected_result)
> +    {
> +#if DEBUG
> +      printf ("ERROR: multiply_add (%lld, %lld, %lld) = ", a, b, c);
> +      print_i128 (result, 0);
> +      printf ("\n does not match expected_result = ");
> +      print_i128 (expected_result, 0);
> +      printf ("\n\n");
> +#else
> +      abort();
> +#endif
> +    }
> +
> +  unsigned long au = 0xFEDCBA9876543210UL;
> +  unsigned long bu = 0x1000000UL;
> +  unsigned long cu = 0x123456UL;
> +  unsigned __int128 expected_resultu = create_i128 (0x0000000000FEDCBAL,
> +						    0x9876543210123456L);
> +
> +  unsigned __int128 resultu = multiply_addu (au, bu, cu);
> +  if (resultu != expected_resultu)
> +    {
> +#if DEBUG
> +      printf ("ERROR: multiply_addu (%llu, %llu, %llu) = ", au, bu, cu);
> +      print_i128 (resultu, 1);
> +      printf ("\n does not match expected_result = ");
> +      print_i128 (expected_resultu, 1);
> +      printf ("\n\n");
> +#else
> +      abort();
> +#endif

Nit: better to add one explicit "return 0;" to avoid possible warning.

BR,
Kewen
> +    }
> +}