[RS6000] Change maddld match_operand from DI to GPR
diff mbox series

Message ID 1561356005-71785-1-git-send-email-helijia@linux.ibm.com
State New
Headers show
Series
  • [RS6000] Change maddld match_operand from DI to GPR
Related show

Commit Message

Li Jia He June 24, 2019, 6 a.m. UTC
Hi,

From PowerPC ISA3.0, the description of `maddld RT, RA.RB, RC` is as follows:
64-bit RA and RB are multiplied and then the RC is signed extend to 128 bits,
and add them together.

We only apply it to 64-bit mode (DI) when implementing maddld.  However, if we
can guarantee that the result of the maddld operation will be limited to 32-bit
mode (SI), we can still apply it to 32-bit mode (SI).

The regression testing for the patch was done on GCC mainline on

    powerpc64le-unknown-linux-gnu (Power 9 LE)

with no regressions.  Is it OK for trunk ?

Thanks,
Lijia He

gcc/ChangeLog
2019-06-24  Li Jia He  <helijia@linux.ibm.com>

	* config/rs6000/rs6000.h (TARGET_MADDLD): Remove the restriction of
	TARGET_POWERPC64.
	* config/rs6000/rs6000.md (maddld): Change maddld match_operand from DI
	to GPR.

gcc/testsuite/ChangeLog

2019-06-24  Li Jia He  <helijia@linux.ibm.com>

	* gcc.target/powerpc/maddld-1.c: New testcase.
---
 gcc/config/rs6000/rs6000.h                  |  2 +-
 gcc/config/rs6000/rs6000.md                 | 10 +++++-----
 gcc/testsuite/gcc.target/powerpc/maddld-1.c | 19 +++++++++++++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/maddld-1.c

Comments

Kewen.Lin June 24, 2019, 6:45 a.m. UTC | #1
Hi Lijia,

on 2019/6/24 下午2:00, Li Jia He wrote:
> Hi,
> 
> From PowerPC ISA3.0, the description of `maddld RT, RA.RB, RC` is as follows:
> 64-bit RA and RB are multiplied and then the RC is signed extend to 128 bits,
> and add them together.
> 
> We only apply it to 64-bit mode (DI) when implementing maddld.  However, if we
> can guarantee that the result of the maddld operation will be limited to 32-bit
> mode (SI), we can still apply it to 32-bit mode (SI).
> 
> The regression testing for the patch was done on GCC mainline on
> 
>     powerpc64le-unknown-linux-gnu (Power 9 LE)
> 
> with no regressions.  Is it OK for trunk ?
> 
> Thanks,
> Lijia He
> 
> gcc/ChangeLog
> 2019-06-24  Li Jia He  <helijia@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.h (TARGET_MADDLD): Remove the restriction of
> 	TARGET_POWERPC64.
> 	* config/rs6000/rs6000.md (maddld): Change maddld match_operand from DI
> 	to GPR.
> 
> gcc/testsuite/ChangeLog
> 
> 2019-06-24  Li Jia He  <helijia@linux.ibm.com>
> 
> 	* gcc.target/powerpc/maddld-1.c: New testcase.
> ---
>  gcc/config/rs6000/rs6000.h                  |  2 +-
>  gcc/config/rs6000/rs6000.md                 | 10 +++++-----
>  gcc/testsuite/gcc.target/powerpc/maddld-1.c | 19 +++++++++++++++++++
>  3 files changed, 25 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/maddld-1.c
> 
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 34fa36b6ed9..f83f19afbba 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -453,7 +453,7 @@ extern int rs6000_vector_align[];
>  #define TARGET_FCTIWUZ	TARGET_POPCNTD
>  #define TARGET_CTZ	TARGET_MODULO
>  #define TARGET_EXTSWSLI	(TARGET_MODULO && TARGET_POWERPC64)
> -#define TARGET_MADDLD	(TARGET_MODULO && TARGET_POWERPC64)
> +#define TARGET_MADDLD	TARGET_MODULO
>  

IMHO, I don't think this removal of TARGET_POWERPC64 is reasonable.
As ISA V3.0, the description of this insn maddld is:
GPR[RT].dword[0] ← Chop(result, 64)

It assumes the GPR has dword, it's a 64-bit specific insn, right?
Your change relaxes it to be adopted on 32-bit.
Although it's fine for powerpc LE since it's always 64-bit, it will
have problems for power9 32bit like AIX?

>  #define TARGET_XSCVDPSPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
>  #define TARGET_XSCVSPDPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 47cbba89443..9122b29e99b 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -3057,11 +3057,11 @@
>    DONE;
>  })
>  
> -(define_insn "*maddld4"
> -  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> -	(plus:DI (mult:DI (match_operand:DI 1 "gpc_reg_operand" "r")
> -			  (match_operand:DI 2 "gpc_reg_operand" "r"))
> -		 (match_operand:DI 3 "gpc_reg_operand" "r")))]
> +(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"))
> +		  (match_operand:GPR 3 "gpc_reg_operand" "r")))]
>    "TARGET_MADDLD"
>    "maddld %0,%1,%2,%3"
>    [(set_attr "type" "mul")])
> diff --git a/gcc/testsuite/gcc.target/powerpc/maddld-1.c b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
> new file mode 100644
> index 00000000000..06f5f5774d4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */


The above target requirement seems useless? since you have
below one which is more specific.


Thanks,
Kewen

> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
> +/* { dg-options "-mdejagnu-cpu=power9 -O2" } */
> +

>
Segher Boessenkool June 24, 2019, 7:19 a.m. UTC | #2
On Mon, Jun 24, 2019 at 02:45:09PM +0800, Kewen.Lin wrote:
> on 2019/6/24 下午2:00, Li Jia He wrote:
> > -#define TARGET_MADDLD	(TARGET_MODULO && TARGET_POWERPC64)
> > +#define TARGET_MADDLD	TARGET_MODULO
> 
> IMHO, I don't think this removal of TARGET_POWERPC64 is reasonable.
> As ISA V3.0, the description of this insn maddld is:
> GPR[RT].dword[0] ← Chop(result, 64)
> 
> It assumes the GPR has dword, it's a 64-bit specific insn, right?
> Your change relaxes it to be adopted on 32-bit.
> Although it's fine for powerpc LE since it's always 64-bit, it will
> have problems for power9 32bit like AIX?

Hi Kewen,

Newer ISAs require 64-bit to be implemented.  There are no optional
64-bit categories anymore.  Since this instruction is enabled for P9
(ISA 3.0) only (that's the TARGET_MODULO), it's fine.

What you are saying is quite true for older CPUs/ISAs though: there you
have to make sure you are targetting a CPU that supports the 64-bit
categories, before using any 64-bit insns.

But those days are gone :-)


Segher
Segher Boessenkool June 24, 2019, 7:38 a.m. UTC | #3
Hi Lijia,

On Mon, Jun 24, 2019 at 01:00:05AM -0500, Li Jia He wrote:
> >From PowerPC ISA3.0, the description of `maddld RT, RA.RB, RC` is as follows:
> 64-bit RA and RB are multiplied and then the RC is signed extend to 128 bits,
> and add them together.
> 
> We only apply it to 64-bit mode (DI) when implementing maddld.  However, if we
> can guarantee that the result of the maddld operation will be limited to 32-bit
> mode (SI), we can still apply it to 32-bit mode (SI).

Great :-)  Just some testcase comments:

> diff --git a/gcc/testsuite/gcc.target/powerpc/maddld-1.c b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
> new file mode 100644
> index 00000000000..06f5f5774d4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */

powerpc* is the default in gcc.target/powerpc, so you can leave it out:

/* { dg-do compile } */

(and that is default itself, but it is good documentation for the target
tests, many of those are run tests).

> +/* { dg-require-effective-target powerpc_p9modulo_ok } */

You don't need this line, it tests if the assembler supports p9.

> +/* { dg-final { scan-assembler-times "maddld " 2 } } */
> +/* { dg-final { scan-assembler-not   "mulld "    } } */
> +/* { dg-final { scan-assembler-not   "add "      } } */

You can easier write this using \m and \M, a bit more exact even:

/* { dg-final { scan-assembler-times {\mmaddld\M} 2 } } */
/* { dg-final { scan-assembler-not   {\mmul} } } */
/* { dg-final { scan-assembler-not   {\madd} } } */

Which allows only the exact mnemonic "maddld", and disallows anything
starting with "mul" or "add".

Okay for trunk, with the testcase improvements please.  Thanks!


Segher
Kewen.Lin June 24, 2019, 7:43 a.m. UTC | #4
on 2019/6/24 下午3:19, Segher Boessenkool wrote:
> On Mon, Jun 24, 2019 at 02:45:09PM +0800, Kewen.Lin wrote:
>> on 2019/6/24 下午2:00, Li Jia He wrote:
>>> -#define TARGET_MADDLD	(TARGET_MODULO && TARGET_POWERPC64)
>>> +#define TARGET_MADDLD	TARGET_MODULO
>>
>> IMHO, I don't think this removal of TARGET_POWERPC64 is reasonable.
>> As ISA V3.0, the description of this insn maddld is:
>> GPR[RT].dword[0] ← Chop(result, 64)
>>
>> It assumes the GPR has dword, it's a 64-bit specific insn, right?
>> Your change relaxes it to be adopted on 32-bit.
>> Although it's fine for powerpc LE since it's always 64-bit, it will
>> have problems for power9 32bit like AIX?
> 
> Hi Kewen,
> 
> Newer ISAs require 64-bit to be implemented.  There are no optional
> 64-bit categories anymore.  Since this instruction is enabled for P9
> (ISA 3.0) only (that's the TARGET_MODULO), it's fine.
> 
> What you are saying is quite true for older CPUs/ISAs though: there you
> have to make sure you are targetting a CPU that supports the 64-bit
> categories, before using any 64-bit insns.
> 
> But those days are gone :-)
> 

Hi Segher,

Good to know that, thanks a lot for the information!  It's fine then.

It sounds like we can have a clean up for some others like 
TARGET_EXTSWSLI. :)


Thanks,
Kewen

> 
> Segher
>
Kewen.Lin June 24, 2019, 7:49 a.m. UTC | #5
on 2019/6/24 下午3:43, Kewen.Lin wrote:
> on 2019/6/24 下午3:19, Segher Boessenkool wrote:
>> On Mon, Jun 24, 2019 at 02:45:09PM +0800, Kewen.Lin wrote:
>>> on 2019/6/24 下午2:00, Li Jia He wrote:
>>>> -#define TARGET_MADDLD	(TARGET_MODULO && TARGET_POWERPC64)
>>>> +#define TARGET_MADDLD	TARGET_MODULO
>>>
>>> IMHO, I don't think this removal of TARGET_POWERPC64 is reasonable.
>>> As ISA V3.0, the description of this insn maddld is:
>>> GPR[RT].dword[0] ← Chop(result, 64)
>>>
>>> It assumes the GPR has dword, it's a 64-bit specific insn, right?
>>> Your change relaxes it to be adopted on 32-bit.
>>> Although it's fine for powerpc LE since it's always 64-bit, it will
>>> have problems for power9 32bit like AIX?
>>
>> Hi Kewen,
>>
>> Newer ISAs require 64-bit to be implemented.  There are no optional
>> 64-bit categories anymore.  Since this instruction is enabled for P9
>> (ISA 3.0) only (that's the TARGET_MODULO), it's fine.
>>
>> What you are saying is quite true for older CPUs/ISAs though: there you
>> have to make sure you are targetting a CPU that supports the 64-bit
>> categories, before using any 64-bit insns.
>>
>> But those days are gone :-)
>>
> 
> Hi Segher,
> 
> Good to know that, thanks a lot for the information!  It's fine then.
> 
> It sounds like we can have a clean up for some others like 
> TARGET_EXTSWSLI. :)
> 

Sorry, maybe not, it's not similar to maddld for 32bit operations.


Thanks,
Kewen

> 
> Thanks,
> Kewen
> 
>>
>> Segher
>>
>
Segher Boessenkool June 24, 2019, 8:02 a.m. UTC | #6
Hi Kewen,

On Mon, Jun 24, 2019 at 03:43:26PM +0800, Kewen.Lin wrote:
> on 2019/6/24 下午3:19, Segher Boessenkool wrote:
> > Newer ISAs require 64-bit to be implemented.  There are no optional
> > 64-bit categories anymore.  Since this instruction is enabled for P9
> > (ISA 3.0) only (that's the TARGET_MODULO), it's fine.
> > 
> > What you are saying is quite true for older CPUs/ISAs though: there you
> > have to make sure you are targetting a CPU that supports the 64-bit
> > categories, before using any 64-bit insns.
> > 
> > But those days are gone :-)
> 
> Good to know that, thanks a lot for the information!  It's fine then.
> 
> It sounds like we can have a clean up for some others like 
> TARGET_EXTSWSLI. :)

Yes, but be careful there!  The insn patterns for this use DImode, which
does not mean the same thing without -mpowerpc64 (it's a register pair
then, not what you want).

And it doesn't make much sense to allow this for SImode as well (using
GPR, perhaps), because the insn just is a shift left for SImode, and we
already have shift left instructions.

So we might want to just directly say "TARGET_MODULO && TARGET_POWERPC64"
in those patterns (TARGET_MODULO is a funny way of saying "p9 or later").


Segher
Kewen.Lin June 24, 2019, 8:08 a.m. UTC | #7
Hi Segher,

on 2019/6/24 下午4:02, Segher Boessenkool wrote:
> Hi Kewen,
> 
> On Mon, Jun 24, 2019 at 03:43:26PM +0800, Kewen.Lin wrote:
>> on 2019/6/24 下午3:19, Segher Boessenkool wrote:
>>> Newer ISAs require 64-bit to be implemented.  There are no optional
>>> 64-bit categories anymore.  Since this instruction is enabled for P9
>>> (ISA 3.0) only (that's the TARGET_MODULO), it's fine.
>>>
>>> What you are saying is quite true for older CPUs/ISAs though: there you
>>> have to make sure you are targetting a CPU that supports the 64-bit
>>> categories, before using any 64-bit insns.
>>>
>>> But those days are gone :-)
>>
>> Good to know that, thanks a lot for the information!  It's fine then.
>>
>> It sounds like we can have a clean up for some others like 
>> TARGET_EXTSWSLI. :)
> 
> Yes, but be careful there!  The insn patterns for this use DImode, which
> does not mean the same thing without -mpowerpc64 (it's a register pair
> then, not what you want).
> 
> And it doesn't make much sense to allow this for SImode as well (using
> GPR, perhaps), because the insn just is a shift left for SImode, and we
> already have shift left instructions.
> 
> So we might want to just directly say "TARGET_MODULO && TARGET_POWERPC64"
> in those patterns (TARGET_MODULO is a funny way of saying "p9 or later").
> 

Thanks for further clarification!  Yes, I agree with you.  I just noticed
that extswsli isn't like maddld and not suitable for SImode.


Thanks,
Kewen

> 
> Segher
>
Segher Boessenkool June 24, 2019, 8:16 a.m. UTC | #8
On Mon, Jun 24, 2019 at 03:49:35PM +0800, Kewen.Lin wrote:
> > It sounds like we can have a clean up for some others like 
> > TARGET_EXTSWSLI. :)
> 
> Sorry, maybe not, it's not similar to maddld for 32bit operations.

Hey, it currently is

(define_insn_and_split "ashdi3_extswsli"
  [(set (match_operand:DI 0 "gpc_reg_operand" "=r,r")
	(ashift:DI
	 (sign_extend:DI (match_operand:SI 1 "reg_or_mem_operand" "r,m"))
	 (match_operand:DI 2 "u6bit_cint_operand" "n,n")))]

so you could just do

(define_insn_and_split "ashdi3_extswsli"
  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r")
	(ashift:GPR
	 (sign_extend:GPR (match_operand:SI 1 "reg_or_mem_operand" "r,m"))
	 (match_operand:GPR 2 "u6bit_cint_operand" "n,n")))]

and that will work, just generate insn patterns that will never match for SI.

But you can also do

(define_insn_and_split "ashl<mode>3_extswsli"
  [(set (match_operand:EXTSI 0 "gpc_reg_operand" "=r,r")
	(ashift:EXTSI
	 (sign_extend:EXTSI (match_operand:SI 1 "reg_or_mem_operand" "r,m"))
	 (match_operand:EXTSI 2 "u6bit_cint_operand" "n,n")))]

and that should work fine, without needing any explicit TARGET_POWERPC64.
But now you need to adjust direct callers of this pattern (which probably
do exist, it is a named pattern (i.e. without *) for a reason ;-)


Segher
Li Jia He June 26, 2019, 5:06 a.m. UTC | #9
On 2019/6/24 3:38 PM, Segher Boessenkool wrote:
> Hi Lijia,
> 
> On Mon, Jun 24, 2019 at 01:00:05AM -0500, Li Jia He wrote:
>> >From PowerPC ISA3.0, the description of `maddld RT, RA.RB, RC` is as follows:
>> 64-bit RA and RB are multiplied and then the RC is signed extend to 128 bits,
>> and add them together.
>>
>> We only apply it to 64-bit mode (DI) when implementing maddld.  However, if we
>> can guarantee that the result of the maddld operation will be limited to 32-bit
>> mode (SI), we can still apply it to 32-bit mode (SI).
> 
> Great :-)  Just some testcase comments:
> 
>> diff --git a/gcc/testsuite/gcc.target/powerpc/maddld-1.c b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
>> new file mode 100644
>> index 00000000000..06f5f5774d4
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
>> @@ -0,0 +1,19 @@
>> +/* { dg-do compile { target { powerpc*-*-* } } } */
> 
> powerpc* is the default in gcc.target/powerpc, so you can leave it out:
> 
> /* { dg-do compile } */
> 
> (and that is default itself, but it is good documentation for the target
> tests, many of those are run tests).
> 
>> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
> 
> You don't need this line, it tests if the assembler supports p9.
> 
>> +/* { dg-final { scan-assembler-times "maddld " 2 } } */
>> +/* { dg-final { scan-assembler-not   "mulld "    } } */
>> +/* { dg-final { scan-assembler-not   "add "      } } */
> 
> You can easier write this using \m and \M, a bit more exact even:
> 
> /* { dg-final { scan-assembler-times {\mmaddld\M} 2 } } *As the file name is madld-1.c, the resulting assembly file contains

    .file   "maddld-1.c"

This will cause the test case to fail.

I will replace it with the following statement
/* { dg-final { scan-assembler-times {\mmaddld\s} 2 } }
> /* { dg-final { scan-assembler-not   {\mmul} } } */
> /* { dg-final { scan-assembler-not   {\madd} } } */
> 
> Which allows only the exact mnemonic "maddld", and disallows anything
> starting with "mul" or "add".
> 
> Okay for trunk, with the testcase improvements please.  Thanks!
> 
> 
> Segher
>

Patch
diff mbox series

diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 34fa36b6ed9..f83f19afbba 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -453,7 +453,7 @@  extern int rs6000_vector_align[];
 #define TARGET_FCTIWUZ	TARGET_POPCNTD
 #define TARGET_CTZ	TARGET_MODULO
 #define TARGET_EXTSWSLI	(TARGET_MODULO && TARGET_POWERPC64)
-#define TARGET_MADDLD	(TARGET_MODULO && TARGET_POWERPC64)
+#define TARGET_MADDLD	TARGET_MODULO
 
 #define TARGET_XSCVDPSPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
 #define TARGET_XSCVSPDPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 47cbba89443..9122b29e99b 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -3057,11 +3057,11 @@ 
   DONE;
 })
 
-(define_insn "*maddld4"
-  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
-	(plus:DI (mult:DI (match_operand:DI 1 "gpc_reg_operand" "r")
-			  (match_operand:DI 2 "gpc_reg_operand" "r"))
-		 (match_operand:DI 3 "gpc_reg_operand" "r")))]
+(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"))
+		  (match_operand:GPR 3 "gpc_reg_operand" "r")))]
   "TARGET_MADDLD"
   "maddld %0,%1,%2,%3"
   [(set_attr "type" "mul")])
diff --git a/gcc/testsuite/gcc.target/powerpc/maddld-1.c b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
new file mode 100644
index 00000000000..06f5f5774d4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/maddld-1.c
@@ -0,0 +1,19 @@ 
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-require-effective-target powerpc_p9modulo_ok } */
+/* { dg-options "-mdejagnu-cpu=power9 -O2" } */
+
+int
+s_madd (int a, int b, int c)
+{
+  return (a * b) + c;
+}
+
+unsigned int
+u_madd (unsigned int a, unsigned int b, unsigned int c)
+{
+  return (a * b) + c;
+}
+
+/* { dg-final { scan-assembler-times "maddld " 2 } } */
+/* { dg-final { scan-assembler-not   "mulld "    } } */
+/* { dg-final { scan-assembler-not   "add "      } } */