| Message ID | bc95278b-a7bd-daa7-e396-fc563451d80b@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Support more short/char to float conversion | expand |
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569792.html BR, Kewen on 2021/5/7 上午10:30, Kewen.Lin via Gcc-patches wrote: > Hi, > > For some cases that when we load unsigned char/short values from > the appropriate unsigned char/short memories and convert them to > double/single precision floating point value, there would be > implicit conversions to int first. It makes GCC not leverage the > P9 instructions lxsibzx/lxsihzx. This patch is to add the related > define_insn_and_split to support this kind of scenario. > > Bootstrapped/regtested on powerpc64le-linux-gnu P9 and > powerpc64-linux-gnu P8. > > Is it ok for trunk? > > BR, > Kewen > ------ > gcc/ChangeLog: > > * config/rs6000/rs6000.md > (floatsi<SFDF:mode>2_lfiwax_<QHI:mode>_mem_zext): New > define_insn_and_split. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/p9-fpcvt-3.c: New test. >
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569792.html BR, Kewen on 2021/5/26 上午11:02, Kewen.Lin via Gcc-patches wrote: > Hi, > > Gentle ping this: > > https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569792.html > > > BR, > Kewen > > on 2021/5/7 上午10:30, Kewen.Lin via Gcc-patches wrote: >> Hi, >> >> For some cases that when we load unsigned char/short values from >> the appropriate unsigned char/short memories and convert them to >> double/single precision floating point value, there would be >> implicit conversions to int first. It makes GCC not leverage the >> P9 instructions lxsibzx/lxsihzx. This patch is to add the related >> define_insn_and_split to support this kind of scenario. >> >> Bootstrapped/regtested on powerpc64le-linux-gnu P9 and >> powerpc64-linux-gnu P8. >> >> Is it ok for trunk? >> >> BR, >> Kewen >> ------ >> gcc/ChangeLog: >> >> * config/rs6000/rs6000.md >> (floatsi<SFDF:mode>2_lfiwax_<QHI:mode>_mem_zext): New >> define_insn_and_split. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/powerpc/p9-fpcvt-3.c: New test. >> >
Hi! On Fri, May 07, 2021 at 10:30:38AM +0800, Kewen.Lin wrote: > For some cases that when we load unsigned char/short values from > the appropriate unsigned char/short memories and convert them to > double/single precision floating point value, there would be > implicit conversions to int first. It makes GCC not leverage the > P9 instructions lxsibzx/lxsihzx. This patch is to add the related > define_insn_and_split to support this kind of scenario. > +/* { dg-final { scan-assembler "lxsibzx" } } */ > +/* { dg-final { scan-assembler "lxsihzx" } } */ > +/* { dg-final { scan-assembler "vextsb2d" } } */ > +/* { dg-final { scan-assembler "vextsh2d" } } */ On my unpatched compiler all these already work, but you say they don't? For the first two I get lxsibzx 33,0,3 vextsb2d 0,1 xscvsxddp 0,32 fadd 1,0,1 blr and lbz 9,0(3) mtvsrwa 0,9 fcfid 0,0 fadd 1,0,1 blr is that different for you? In either case, use \m and \M please. > +/* { dg-final { scan-assembler-not "mfvsrd" } } */ > +/* { dg-final { scan-assembler-not "mfvsrwz" } } */ > +/* { dg-final { scan-assembler-not "mtvsrd" } } */ > +/* { dg-final { scan-assembler-not "mtvsrwa" } } */ > +/* { dg-final { scan-assembler-not "mtvsrwz" } } */ Here as well, or you could just do /* { dg-final { scan-assembler-not "\mm[tf]vsr" } } */ in this case, since no VSR<->GPR moves should happen at all. Segher
Hi Segher, Thanks for the review! on 2021/6/10 上午7:23, Segher Boessenkool wrote: > Hi! > > On Fri, May 07, 2021 at 10:30:38AM +0800, Kewen.Lin wrote: >> For some cases that when we load unsigned char/short values from >> the appropriate unsigned char/short memories and convert them to >> double/single precision floating point value, there would be >> implicit conversions to int first. It makes GCC not leverage the >> P9 instructions lxsibzx/lxsihzx. This patch is to add the related >> define_insn_and_split to support this kind of scenario. > >> +/* { dg-final { scan-assembler "lxsibzx" } } */ >> +/* { dg-final { scan-assembler "lxsihzx" } } */ >> +/* { dg-final { scan-assembler "vextsb2d" } } */ >> +/* { dg-final { scan-assembler "vextsh2d" } } */ > > On my unpatched compiler all these already work, but you say they don't? > Sorry for the confusion, the patch is to handle the unsigned char and short but the test case also has the test coverage on signed char and short, which follows the existing cases p9-fpcvt-{1,2}.c. As you stated, the signed part of cases are fine. > For the first two I get > lxsibzx 33,0,3 > vextsb2d 0,1 > xscvsxddp 0,32 > fadd 1,0,1 > blr > and > lbz 9,0(3) > mtvsrwa 0,9 > fcfid 0,0 > fadd 1,0,1 > blr > is that different for you? > I got the same output without the patch, applying the patch the second one becomes into: lxsibzx 0,0,3 fcfid 0,0 fadd 1,0,1 blr > In either case, use \m and \M please. > Fixed. >> +/* { dg-final { scan-assembler-not "mfvsrd" } } */ >> +/* { dg-final { scan-assembler-not "mfvsrwz" } } */ >> +/* { dg-final { scan-assembler-not "mtvsrd" } } */ >> +/* { dg-final { scan-assembler-not "mtvsrwa" } } */ >> +/* { dg-final { scan-assembler-not "mtvsrwz" } } */ > > Here as well, or you could just do > > /* { dg-final { scan-assembler-not "\mm[tf]vsr" } } */ > > in this case, since no VSR<->GPR moves should happen at all. > Thanks, updated accordingly. The patch v2 is attached, does it look better? BR, Kewen ------ gcc/ChangeLog: * config/rs6000/rs6000.md (floatsi<SFDF:mode>2_lfiwax_<QHI:mode>_mem_zext): New define_insn_and_split. gcc/testsuite/ChangeLog: * gcc.target/powerpc/p9-fpcvt-3.c: New test. diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index 3f59b544f6a..0574e10f923 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -5524,6 +5524,27 @@ (define_insn_and_split "floatsi<mode>2_lfiwax_mem" [(set_attr "length" "8") (set_attr "type" "fpload")]) +(define_insn_and_split "floatsi<SFDF:mode>2_lfiwax_<QHI:mode>_mem_zext" + [(set (match_operand:SFDF 0 "gpc_reg_operand" "=d,<Fv>") + (float:SFDF + (zero_extend:SI + (match_operand:QHI 1 "indexed_or_indirect_operand" "Z,Z")))) + (clobber (match_scratch:DI 2 "=d,wa"))] + "TARGET_HARD_FLOAT && <SI_CONVERT_FP> && TARGET_P9_VECTOR + && TARGET_POWERPC64 && TARGET_DIRECT_MOVE" + "#" + "&& 1" + [(pc)] +{ + if (GET_CODE (operands[2]) == SCRATCH) + operands[2] = gen_reg_rtx (DImode); + emit_insn (gen_zero_extendhidi2 (operands[2], operands[1])); + emit_insn (gen_floatdi<SFDF:mode>2 (operands[0], operands[2])); + DONE; +} + [(set_attr "length" "8") + (set_attr "type" "fpload")]) + (define_insn "lfiwzx" [(set (match_operand:DI 0 "gpc_reg_operand" "=d,wa,wa,wa") (unspec:DI [(match_operand:SI 1 "reg_or_indexed_operand" "Z,Z,r,wa")] diff --git a/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c b/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c new file mode 100644 index 00000000000..19701c84add --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c @@ -0,0 +1,23 @@ +/* { dg-do compile { target lp64 } } */ +/* { dg-require-effective-target powerpc_p9vector_ok } */ +/* { dg-options "-mdejagnu-cpu=power9 -O2" } */ + +/* Note that for unsigned cases, the differences from those ones in + p9-fpcvt-2.c is that they will be converted to int implicitly first + and then to floating point. */ + +double sc_df (signed char *p, double df) { return *p + df; } +double uc_df (unsigned char *p, double df) { return *p + df; } +double ss_df (signed short *p, double df) { return *p + df; } +double us_df (unsigned short *p, double df) { return *p + df; } + +float sc_sf (signed char *p, float sf) { return *p + sf; } +float uc_sf (unsigned char *p, float sf) { return *p + sf; } +float ss_sf (signed short *p, float sf) { return *p + sf; } +float us_sf (unsigned short *p, float sf) { return *p + sf; } + +/* { dg-final { scan-assembler {\mlxsibzx\M} } } */ +/* { dg-final { scan-assembler {\mlxsihzx\M} } } */ +/* { dg-final { scan-assembler {\mvextsb2d\M} } } */ +/* { dg-final { scan-assembler {\mvextsh2d\M} } } */ +/* { dg-final { scan-assembler-not {\mm[tf]vsr} } } */
Hi!
On Thu, Jun 10, 2021 at 05:32:23PM +0800, Kewen.Lin wrote:
> +/* { dg-do compile { target lp64 } } */
One final thing: what requires lp64 here? Could you try without please?
Okay for trunk with that considered. Thanks!
Segher
Hi Segher, on 2021/6/10 下午6:58, Segher Boessenkool wrote: > Hi! > > On Thu, Jun 10, 2021 at 05:32:23PM +0800, Kewen.Lin wrote: >> +/* { dg-do compile { target lp64 } } */ > > One final thing: what requires lp64 here? Could you try without please? > The lp64 is required for lxsi[bh]zx and mvexts[bh]2d, without it -m32 testing will have some failures as below: FAIL: gcc.target/powerpc/p9-fpcvt-3.c scan-assembler \\mlxsibzx\\M FAIL: gcc.target/powerpc/p9-fpcvt-3.c scan-assembler \\mlxsihzx\\M FAIL: gcc.target/powerpc/p9-fpcvt-3.c scan-assembler \\\\M FAIL: gcc.target/powerpc/p9-fpcvt-3.c scan-assembler \\mvextsh2d\\M > Okay for trunk with that considered. Thanks! > Thanks! It's committed in r12-1378. BR, Kewen
On Fri, Jun 11, 2021 at 08:45:53PM +0800, Kewen.Lin wrote: > on 2021/6/10 下午6:58, Segher Boessenkool wrote: > > On Thu, Jun 10, 2021 at 05:32:23PM +0800, Kewen.Lin wrote: > >> +/* { dg-do compile { target lp64 } } */ > > > > One final thing: what requires lp64 here? Could you try without please? > > The lp64 is required for lxsi[bh]zx and mvexts[bh]2d, without it > -m32 testing will have some failures as below: Ah, those extends of course (and the loads fall out from that). I should have seen that, thanks :-) Segher
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index c8cdc42533c..3ac7ed20852 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -5504,6 +5504,28 @@ (define_insn_and_split "floatsi<mode>2_lfiwax_mem" [(set_attr "length" "8") (set_attr "type" "fpload")]) +(define_insn_and_split "floatsi<SFDF:mode>2_lfiwax_<QHI:mode>_mem_zext" + [(set (match_operand:SFDF 0 "gpc_reg_operand" "=d,<Fv>") + (float:SFDF + (zero_extend:SI + (match_operand:QHI 1 "indexed_or_indirect_operand" "Z,Z")))) + (clobber (match_scratch:DI 2 "=d,wa"))] + "TARGET_HARD_FLOAT && <SI_CONVERT_FP> && TARGET_P9_VECTOR + && TARGET_POWERPC64 && TARGET_DIRECT_MOVE" + "#" + "&& 1" + [(pc)] +{ + operands[1] = rs6000_force_indexed_or_indirect_mem (operands[1]); + if (GET_CODE (operands[2]) == SCRATCH) + operands[2] = gen_reg_rtx (DImode); + emit_insn (gen_zero_extendhidi2 (operands[2], operands[1])); + emit_insn (gen_floatdi<SFDF:mode>2 (operands[0], operands[2])); + DONE; +} + [(set_attr "length" "8") + (set_attr "type" "fpload")]) + (define_insn "lfiwzx" [(set (match_operand:DI 0 "gpc_reg_operand" "=d,wa,wa,wa") (unspec:DI [(match_operand:SI 1 "reg_or_indexed_operand" "Z,Z,r,wa")] diff --git a/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c b/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c new file mode 100644 index 00000000000..d3bbe36b759 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c @@ -0,0 +1,27 @@ +/* { dg-do compile { target lp64 } } */ +/* { dg-require-effective-target powerpc_p9vector_ok } */ +/* { dg-options "-mdejagnu-cpu=power9 -O2" } */ + +/* Note that for unsigned cases, the differences from those ones in + p9-fpcvt-2.c is that they will be converted to int implicitly first + and then to floating point. */ + +double sc_df (signed char *p, double df) { return *p + df; } +double uc_df (unsigned char *p, double df) { return *p + df; } +double ss_df (signed short *p, double df) { return *p + df; } +double us_df (unsigned short *p, double df) { return *p + df; } + +float sc_sf (signed char *p, float sf) { return *p + sf; } +float uc_sf (unsigned char *p, float sf) { return *p + sf; } +float ss_sf (signed short *p, float sf) { return *p + sf; } +float us_sf (unsigned short *p, float sf) { return *p + sf; } + +/* { dg-final { scan-assembler "lxsibzx" } } */ +/* { dg-final { scan-assembler "lxsihzx" } } */ +/* { dg-final { scan-assembler "vextsb2d" } } */ +/* { dg-final { scan-assembler "vextsh2d" } } */ +/* { dg-final { scan-assembler-not "mfvsrd" } } */ +/* { dg-final { scan-assembler-not "mfvsrwz" } } */ +/* { dg-final { scan-assembler-not "mtvsrd" } } */ +/* { dg-final { scan-assembler-not "mtvsrwa" } } */ +/* { dg-final { scan-assembler-not "mtvsrwz" } } */