[PATCH] rs6000: Support more short/char to float conversion

[PATCH] rs6000: Support more short/char to float conversion

[Kewen.Lin]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

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.

[-- Attachment #2: 0003-rs6000-Support-more-short-char-to-float-conversion.patch --]
[-- Type: text/plain, Size: 3074 bytes --]

---
 gcc/config/rs6000/rs6000.md                   | 22 +++++++++++++++
 gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c | 27 +++++++++++++++++++
 2 files changed, 49 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/p9-fpcvt-3.c

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

PING^1 [PATCH] rs6000: Support more short/char to float conversion

[Kewen.Lin]

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.
> 

PING^2 [PATCH] rs6000: Support more short/char to float conversion

[Kewen.Lin]

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.
>>
> 

Re: [PATCH] rs6000: Support more short/char to float conversion

[Segher Boessenkool]

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

Re: [PATCH] rs6000: Support more short/char to float conversion

[Kewen.Lin]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

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.

[-- Attachment #2: conv_v2.diff --]
[-- Type: text/plain, Size: 2551 bytes --]

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}   } } */

Re: [PATCH] rs6000: Support more short/char to float conversion

[Segher Boessenkool]

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

Re: [PATCH] rs6000: Support more short/char to float conversion

[Kewen.Lin]

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

Re: [PATCH] rs6000: Support more short/char to float conversion

[Segher Boessenkool]

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