| 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 | expand |
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" } */ > + >
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
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
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 >
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 >> >
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
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 >
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
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 >
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 " } } */
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