[PING][PATCH][Aarch64] Improve int<->FP conversions

[PING][PATCH][Aarch64] Improve int<->FP conversions

[Michael Collison]

Ping. Original patch posted here:

https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00808.html

Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

[James Greenhalgh]

On Fri, Sep 29, 2017 at 10:42:14PM +0100, Michael Collison wrote:
> Ping. Original patch posted here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00808.html

I don't see a patch attached there to review, nor can I see an earlier mail
threaded to it with the patch attached. Can you help?

Thanks,
James

RE: [PING][PATCH][Aarch64] Improve int<->FP conversions

[Michael Collison]

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

Sorry. Here is the patch.

-----Original Message-----
From: James Greenhalgh [mailto:james.greenhalgh@arm.com] 
Sent: Friday, September 29, 2017 3:57 PM
To: Michael Collison <Michael.Collison@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>
Subject: Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

On Fri, Sep 29, 2017 at 10:42:14PM +0100, Michael Collison wrote:
> Ping. Original patch posted here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00808.html

I don't see a patch attached there to review, nor can I see an earlier mail threaded to it with the patch attached. Can you help?

Thanks,
James


[-- Attachment #2: pr6527.patch --]
[-- Type: application/octet-stream, Size: 3281 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index c1bca07..e7226a2 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4816,11 +4816,29 @@
   [(set_attr "type" "f_cvt")]
 )
 
-(define_insn "<optab>_trunc<GPF_F16:mode><GPI:mode>2"
+(define_insn "<optab>_trunc<vf><GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=?r,w")
+	(FIXUORS:GPI (match_operand:<VF> 1 "register_operand" "w,w")))]
+  "TARGET_FLOAT"
+  "@
+   fcvtz<su>\t%<w>0, %<s>1
+   fcvtz<su>\t%<s>0, %<s>1"
+  [(set_attr "type" "f_cvtf2i,neon_fp_to_int_s")]
+)
+
+(define_insn "<optab>_trunchf<GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+	(FIXUORS:GPI (match_operand:HF 1 "register_operand" "w")))]
+  "TARGET_FP_F16INST"
+  "fcvtz<su>\t%<w>0, %h1"
+  [(set_attr "type" "f_cvtf2i")]
+)
+
+(define_insn "<optab>_trunc<vgp><GPI:mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
-	(FIXUORS:GPI (match_operand:GPF_F16 1 "register_operand" "w")))]
+	(FIXUORS:GPI (match_operand:<VGP> 1 "register_operand" "w")))]
   "TARGET_FLOAT"
-  "fcvtz<su>\t%<GPI:w>0, %<GPF_F16:s>1"
+  "fcvtz<su>\t%<w>0, %<wv>1"
   [(set_attr "type" "f_cvtf2i")]
 )
 
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index cceb575..166a044 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -391,6 +391,9 @@
 (define_mode_attr w1 [(HF "w") (SF "w") (DF "x")])
 (define_mode_attr w2 [(HF "x") (SF "x") (DF "w")])
 
+;; For inequal width float to int conversion
+(define_mode_attr wv [(DI "s") (SI "d")])
+
 (define_mode_attr short_mask [(HI "65535") (QI "255")])
 
 ;; For constraints used in scalar immediate vector moves
@@ -399,6 +402,14 @@
 ;; For doubling width of an integer mode
 (define_mode_attr DWI [(QI "HI") (HI "SI") (SI "DI") (DI "TI")])
 
+(define_mode_attr vf [(SI "sf") (DI "df")])
+
+(define_mode_attr VF [(SI "SF") (DI "DF")])
+
+(define_mode_attr vgp [(SI "df") (DI "sf")])
+
+(define_mode_attr VGP [(SI "DF") (DI "SF")])
+
 ;; For scalar usage of vector/FP registers
 (define_mode_attr v [(QI "b") (HI "h") (SI "s") (DI "d")
 		    (HF  "h") (SF "s") (DF "d")
@@ -432,7 +443,7 @@
 (define_mode_attr vas [(DI "") (SI ".2s")])
 
 ;; Map a floating point mode to the appropriate register name prefix
-(define_mode_attr s [(HF "h") (SF "s") (DF "d")])
+(define_mode_attr s [(HF "h") (SF "s") (DF "d") (SI "s") (DI "d")])
 
 ;; Give the length suffix letter for a sign- or zero-extension.
 (define_mode_attr size [(QI "b") (HI "h") (SI "w")])
diff --git a/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
new file mode 100644
index 0000000..0441458
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+float
+f1 (float x)
+{
+  int y = x;
+
+  return (float) y;
+}
+
+double
+f2 (double x)
+{
+  long y = x;
+
+  return (double) y;
+}
+
+/* { dg-final { scan-assembler "fcvtzs\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzs\\td\[0-9\]+, d\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\td\[0-9\]+, d\[0-9\]+" } } */
-- 
1.9.1

Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

[James Greenhalgh]

On Sun, Oct 01, 2017 at 02:07:57AM +0100, Michael Collison wrote:
> Sorry. Here is the patch.

I think this needs a small amount fo rework in iterators.md - the names
you've used don't follow conventions in that file (e.g. "V" normally has
something to do with vectors) so could do with patching up.


> -(define_insn "<optab>_trunc<GPF_F16:mode><GPI:mode>2"
> +(define_insn "<optab>_trunc<vf><GPI:mode>2"
> +  [(set (match_operand:GPI 0 "register_operand" "=?r,w")
> +	(FIXUORS:GPI (match_operand:<VF> 1 "register_operand" "w,w")))]
> +  "TARGET_FLOAT"
> +  "@
> +   fcvtz<su>\t%<w>0, %<s>1
> +   fcvtz<su>\t%<s>0, %<s>1"
> +  [(set_attr "type" "f_cvtf2i,neon_fp_to_int_s")]
> +)

So the point here is that we need to fork the pattern for two reasons.
Before we were iterating over both floating-point modes as the input to any
integer-modes as output. Because only the same-sized instructions have
vector-register to vector-register forms we need two patterns. One for
same-size, one for cross-size. And one more special pattern for HFmode.

This makes sense to me. A comment explaining why we need the two patterns
would be even easier to read.

This pattern gives us SFmode to SImode and DFmode to DImode.

> +(define_insn "<optab>_trunchf<GPI:mode>2"
> +  [(set (match_operand:GPI 0 "register_operand" "=r")
> +	(FIXUORS:GPI (match_operand:HF 1 "register_operand" "w")))]
> +  "TARGET_FP_F16INST"
> +  "fcvtz<su>\t%<w>0, %h1"
> +  [(set_attr "type" "f_cvtf2i")]

This pattern we need for HFmode to SImode.

> +(define_insn "<optab>_trunc<vgp><GPI:mode>2"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
> -	(FIXUORS:GPI (match_operand:GPF_F16 1 "register_operand" "w")))]
> +	(FIXUORS:GPI (match_operand:<VGP> 1 "register_operand" "w")))]
>    "TARGET_FLOAT"
> -  "fcvtz<su>\t%<GPI:w>0, %<GPF_F16:s>1"
> +  "fcvtz<su>\t%<w>0, %<wv>1"
>    [(set_attr "type" "f_cvtf2i")]
>  )

And this pattern gives SFmode to DImode and DFmode to SImode.

Comments would definitely help here.

> diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
> index cceb575..166a044 100644
> --- a/gcc/config/aarch64/iterators.md
> +++ b/gcc/config/aarch64/iterators.md
> @@ -391,6 +391,9 @@
>  (define_mode_attr w1 [(HF "w") (SF "w") (DF "x")])
>  (define_mode_attr w2 [(HF "x") (SF "x") (DF "w")])
>  
> +;; For inequal width float to int conversion
> +(define_mode_attr wv [(DI "s") (SI "d")])

Could you invent a more descriptive name for this?

> +
>  (define_mode_attr short_mask [(HI "65535") (QI "255")])
>  
>  ;; For constraints used in scalar immediate vector moves
> @@ -399,6 +402,14 @@
>  ;; For doubling width of an integer mode
>  (define_mode_attr DWI [(QI "HI") (HI "SI") (SI "DI") (DI "TI")])
>  
> +(define_mode_attr vf [(SI "sf") (DI "df")])
> +
> +(define_mode_attr VF [(SI "SF") (DI "DF")])

These two are fcvt_target and FCVT_TARGET ?

> +(define_mode_attr vgp [(SI "df") (DI "sf")])
> +
> +(define_mode_attr VGP [(SI "DF") (DI "SF")])

These names don't make sense to me - V is usually vector and GP sounds
like general purpose. Maybe something like fcvt_change_mode ? Try to be
more descriptive.

> +
>  ;; For scalar usage of vector/FP registers
>  (define_mode_attr v [(QI "b") (HI "h") (SI "s") (DI "d")
>  		    (HF  "h") (SF "s") (DF "d")
> @@ -432,7 +443,7 @@
>  (define_mode_attr vas [(DI "") (SI ".2s")])
>  
>  ;; Map a floating point mode to the appropriate register name prefix

This comment is out of date after your changes, please update it.

> -(define_mode_attr s [(HF "h") (SF "s") (DF "d")])
> +(define_mode_attr s [(HF "h") (SF "s") (DF "d") (SI "s") (DI "d")])
>  
>  ;; Give the length suffix letter for a sign- or zero-extension.
>  (define_mode_attr size [(QI "b") (HI "h") (SI "w")])


Thanks,
James

RE: [PING][PATCH][Aarch64] Improve int<->FP conversions

[Michael Collison]

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

Patch updated with all comments from James.

Bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?

2017-10-15  Michael Collison  <michael.collison@arm.com>

	* config/aarch64/aarch64.md(<optab>_trunc><vf><GPI:mode>2):
	New pattern.
	(<optab>_trunchf<GPI:mode>2: New pattern.
	(<optab>_trunc<vgp><GPI:mode>2: New pattern.
	* config/aarch64/iterators.md (fpw): New mode attribute.
	(fcvt_change_mode, FCVT_CHANGE_MODE): New mode attributes.
	(s): Update attribute with SImode and DImode prefixes.
	* testsuite/gcc.target/aarch64/fix_trunc1.c: New testcase.

-----Original Message-----
From: James Greenhalgh [mailto:james.greenhalgh@arm.com] 
Sent: Wednesday, October 4, 2017 2:39 AM
To: Michael Collison <Michael.Collison@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>
Subject: Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

On Sun, Oct 01, 2017 at 02:07:57AM +0100, Michael Collison wrote:
> Sorry. Here is the patch.

I think this needs a small amount fo rework in iterators.md - the names you've used don't follow conventions in that file (e.g. "V" normally has something to do with vectors) so could do with patching up.


> -(define_insn "<optab>_trunc<GPF_F16:mode><GPI:mode>2"
> +(define_insn "<optab>_trunc<vf><GPI:mode>2"
> +  [(set (match_operand:GPI 0 "register_operand" "=?r,w")
> +	(FIXUORS:GPI (match_operand:<VF> 1 "register_operand" "w,w")))]
> +  "TARGET_FLOAT"
> +  "@
> +   fcvtz<su>\t%<w>0, %<s>1
> +   fcvtz<su>\t%<s>0, %<s>1"
> +  [(set_attr "type" "f_cvtf2i,neon_fp_to_int_s")]
> +)

So the point here is that we need to fork the pattern for two reasons.
Before we were iterating over both floating-point modes as the input to any integer-modes as output. Because only the same-sized instructions have vector-register to vector-register forms we need two patterns. One for same-size, one for cross-size. And one more special pattern for HFmode.

This makes sense to me. A comment explaining why we need the two patterns would be even easier to read.

This pattern gives us SFmode to SImode and DFmode to DImode.

> +(define_insn "<optab>_trunchf<GPI:mode>2"
> +  [(set (match_operand:GPI 0 "register_operand" "=r")
> +	(FIXUORS:GPI (match_operand:HF 1 "register_operand" "w")))]
> +  "TARGET_FP_F16INST"
> +  "fcvtz<su>\t%<w>0, %h1"
> +  [(set_attr "type" "f_cvtf2i")]

This pattern we need for HFmode to SImode.

> +(define_insn "<optab>_trunc<vgp><GPI:mode>2"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
> -	(FIXUORS:GPI (match_operand:GPF_F16 1 "register_operand" "w")))]
> +	(FIXUORS:GPI (match_operand:<VGP> 1 "register_operand" "w")))]
>    "TARGET_FLOAT"
> -  "fcvtz<su>\t%<GPI:w>0, %<GPF_F16:s>1"
> +  "fcvtz<su>\t%<w>0, %<wv>1"
>    [(set_attr "type" "f_cvtf2i")]
>  )

And this pattern gives SFmode to DImode and DFmode to SImode.

Comments would definitely help here.

> diff --git a/gcc/config/aarch64/iterators.md 
> b/gcc/config/aarch64/iterators.md index cceb575..166a044 100644
> --- a/gcc/config/aarch64/iterators.md
> +++ b/gcc/config/aarch64/iterators.md
> @@ -391,6 +391,9 @@
>  (define_mode_attr w1 [(HF "w") (SF "w") (DF "x")])  (define_mode_attr 
> w2 [(HF "x") (SF "x") (DF "w")])
>  
> +;; For inequal width float to int conversion (define_mode_attr wv 
> +[(DI "s") (SI "d")])

Could you invent a more descriptive name for this?

> +
>  (define_mode_attr short_mask [(HI "65535") (QI "255")])
>  
>  ;; For constraints used in scalar immediate vector moves @@ -399,6 
> +402,14 @@  ;; For doubling width of an integer mode  
> (define_mode_attr DWI [(QI "HI") (HI "SI") (SI "DI") (DI "TI")])
>  
> +(define_mode_attr vf [(SI "sf") (DI "df")])
> +
> +(define_mode_attr VF [(SI "SF") (DI "DF")])

These two are fcvt_target and FCVT_TARGET ?

> +(define_mode_attr vgp [(SI "df") (DI "sf")])
> +
> +(define_mode_attr VGP [(SI "DF") (DI "SF")])

These names don't make sense to me - V is usually vector and GP sounds like general purpose. Maybe something like fcvt_change_mode ? Try to be more descriptive.

> +
>  ;; For scalar usage of vector/FP registers  (define_mode_attr v [(QI 
> "b") (HI "h") (SI "s") (DI "d")
>  		    (HF  "h") (SF "s") (DF "d")
> @@ -432,7 +443,7 @@
>  (define_mode_attr vas [(DI "") (SI ".2s")])
>  
>  ;; Map a floating point mode to the appropriate register name prefix

This comment is out of date after your changes, please update it.

> -(define_mode_attr s [(HF "h") (SF "s") (DF "d")])
> +(define_mode_attr s [(HF "h") (SF "s") (DF "d") (SI "s") (DI "d")])
>  
>  ;; Give the length suffix letter for a sign- or zero-extension.
>  (define_mode_attr size [(QI "b") (HI "h") (SI "w")])


Thanks,
James


[-- Attachment #2: pr6527v4.patch --]
[-- Type: application/octet-stream, Size: 3662 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index c1bca07..12d9242 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4816,11 +4816,37 @@
   [(set_attr "type" "f_cvt")]
 )
 
-(define_insn "<optab>_trunc<GPF_F16:mode><GPI:mode>2"
+;; Convert SF -> SI or DF -> DI while preferring w = w register constraints
+;; and making r = w more expensive
+
+(define_insn "<optab>_trunc<fcvt_target><GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=?r,w")
+	(FIXUORS:GPI (match_operand:<FCVT_TARGET> 1 "register_operand" "w,w")))]
+  "TARGET_FLOAT"
+  "@
+   fcvtz<su>\t%<w>0, %<s>1
+   fcvtz<su>\t%<s>0, %<s>1"
+  [(set_attr "type" "f_cvtf2i,neon_fp_to_int_s")]
+)
+
+;; Convert HF -> SI or DI
+
+(define_insn "<optab>_trunchf<GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+	(FIXUORS:GPI (match_operand:HF 1 "register_operand" "w")))]
+  "TARGET_FP_F16INST"
+  "fcvtz<su>\t%<w>0, %h1"
+  [(set_attr "type" "f_cvtf2i")]
+)
+
+;; Convert DF -> SI or SF -> DI which can only be accomplished with
+;; input in a fp register and output in a integer register
+
+(define_insn "<optab>_trunc<fcvt_change_mode><GPI:mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
-	(FIXUORS:GPI (match_operand:GPF_F16 1 "register_operand" "w")))]
+	(FIXUORS:GPI (match_operand:<FCVT_CHANGE_MODE> 1 "register_operand" "w")))]
   "TARGET_FLOAT"
-  "fcvtz<su>\t%<GPI:w>0, %<GPF_F16:s>1"
+  "fcvtz<su>\t%<w>0, %<fpw>1"
   [(set_attr "type" "f_cvtf2i")]
 )
 
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index cceb575..eba04c1 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -391,6 +391,9 @@
 (define_mode_attr w1 [(HF "w") (SF "w") (DF "x")])
 (define_mode_attr w2 [(HF "x") (SF "x") (DF "w")])
 
+;; For width of fp registers in fcvt instruction
+(define_mode_attr fpw [(DI "s") (SI "d")])
+
 (define_mode_attr short_mask [(HI "65535") (QI "255")])
 
 ;; For constraints used in scalar immediate vector moves
@@ -399,6 +402,10 @@
 ;; For doubling width of an integer mode
 (define_mode_attr DWI [(QI "HI") (HI "SI") (SI "DI") (DI "TI")])
 
+(define_mode_attr fcvt_change_mode [(SI "df") (DI "sf")])
+
+(define_mode_attr FCVT_CHANGE_MODE [(SI "DF") (DI "SF")])
+
 ;; For scalar usage of vector/FP registers
 (define_mode_attr v [(QI "b") (HI "h") (SI "s") (DI "d")
 		    (HF  "h") (SF "s") (DF "d")
@@ -431,8 +438,8 @@
 (define_mode_attr rtn [(DI "d") (SI "")])
 (define_mode_attr vas [(DI "") (SI ".2s")])
 
-;; Map a floating point mode to the appropriate register name prefix
-(define_mode_attr s [(HF "h") (SF "s") (DF "d")])
+;; Map a floating point or integer mode to the appropriate register name prefix
+(define_mode_attr s [(HF "h") (SF "s") (DF "d") (SI "s") (DI "d")])
 
 ;; Give the length suffix letter for a sign- or zero-extension.
 (define_mode_attr size [(QI "b") (HI "h") (SI "w")])
diff --git a/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
new file mode 100644
index 0000000..0441458
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+float
+f1 (float x)
+{
+  int y = x;
+
+  return (float) y;
+}
+
+double
+f2 (double x)
+{
+  long y = x;
+
+  return (double) y;
+}
+
+/* { dg-final { scan-assembler "fcvtzs\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzs\\td\[0-9\]+, d\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\td\[0-9\]+, d\[0-9\]+" } } */
-- 
1.9.1

Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

[James Greenhalgh]

On Tue, Oct 17, 2017 at 01:17:04AM +0100, Michael Collison wrote:
> Patch updated with all comments from James.

OK with an appropriate ChangeLog and assuming it has been tested as
required.

Thanks,
James

Reviewed-by: <james.greenhalgh@arm.com>

RE: [PING][PATCH][Aarch64] Improve int<->FP conversions

[Michael Collison]

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

James,

The patch was test as required. However when I tested with the latest trunk there were two test failures that need to be updated because of my patch. 

The files gcc.target/aarch64/vect-vcvt.c was failing because the scan-assembler directives assumed the destination register was assumed to be an integer register. With my patch the destination can be an integer or fp register.

I fixed the failures and bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?

2017-09-02  Michael Collison  <michael.collison@arm.com>

	* config/aarch64/aarch64.md(<optab>_trunc><vf><GPI:mode>2):
	New pattern.
	(<optab>_trunchf<GPI:mode>2: New pattern.
	(<optab>_trunc<vgp><GPI:mode>2: New pattern.
	* config/aarch64/iterators.md (wv): New mode attribute.
	(vf, VF): New mode attributes.
	(vgp, VGP): New mode attributes.
	(s): Update attribute with SImode and DImode prefixes.
	* testsuite/gcc.target/aarch64/fix_trunc1.c: New testcase.
	* testsuite/gcc.target/aarch64/vect-vcvt.c: Fix scan-assembler
	directives to allow float or integer destination registers for
	fcvtz[su].

-----Original Message-----
From: James Greenhalgh [mailto:james.greenhalgh@arm.com] 
Sent: Monday, October 23, 2017 9:43 AM
To: Michael Collison <Michael.Collison@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>
Subject: Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

On Tue, Oct 17, 2017 at 01:17:04AM +0100, Michael Collison wrote:
> Patch updated with all comments from James.

OK with an appropriate ChangeLog and assuming it has been tested as required.

Thanks,
James

Reviewed-by: <james.greenhalgh@arm.com>



[-- Attachment #2: gnutools-6527-v6.patch --]
[-- Type: application/octet-stream, Size: 5070 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 389f2f9..4dd1b07 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4849,11 +4849,37 @@
   [(set_attr "type" "f_cvt")]
 )
 
-(define_insn "<optab>_trunc<GPF_F16:mode><GPI:mode>2"
+;; Convert SF -> SI or DF -> DI while preferring w = w register constraints
+;; and making r = w more expensive
+
+(define_insn "<optab>_trunc<fcvt_target><GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=?r,w")
+	(FIXUORS:GPI (match_operand:<FCVT_TARGET> 1 "register_operand" "w,w")))]
+  "TARGET_FLOAT"
+  "@
+   fcvtz<su>\t%<w>0, %<s>1
+   fcvtz<su>\t%<s>0, %<s>1"
+  [(set_attr "type" "f_cvtf2i,neon_fp_to_int_s")]
+)
+
+;; Convert HF -> SI or DI
+
+(define_insn "<optab>_trunchf<GPI:mode>2"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+	(FIXUORS:GPI (match_operand:HF 1 "register_operand" "w")))]
+  "TARGET_FP_F16INST"
+  "fcvtz<su>\t%<w>0, %h1"
+  [(set_attr "type" "f_cvtf2i")]
+)
+
+;; Convert DF -> SI or SF -> DI which can only be accomplished with
+;; input in a fp register and output in a integer register
+
+(define_insn "<optab>_trunc<fcvt_change_mode><GPI:mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
-	(FIXUORS:GPI (match_operand:GPF_F16 1 "register_operand" "w")))]
+	(FIXUORS:GPI (match_operand:<FCVT_CHANGE_MODE> 1 "register_operand" "w")))]
   "TARGET_FLOAT"
-  "fcvtz<su>\t%<GPI:w>0, %<GPF_F16:s>1"
+  "fcvtz<su>\t%<w>0, %<fpw>1"
   [(set_attr "type" "f_cvtf2i")]
 )
 
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index 48cedbe..68da316 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -398,6 +398,9 @@
 (define_mode_attr w1 [(HF "w") (SF "w") (DF "x")])
 (define_mode_attr w2 [(HF "x") (SF "x") (DF "w")])
 
+;; For width of fp registers in fcvt instruction
+(define_mode_attr fpw [(DI "s") (SI "d")])
+
 (define_mode_attr short_mask [(HI "65535") (QI "255")])
 
 ;; For constraints used in scalar immediate vector moves
@@ -406,6 +409,10 @@
 ;; For doubling width of an integer mode
 (define_mode_attr DWI [(QI "HI") (HI "SI") (SI "DI") (DI "TI")])
 
+(define_mode_attr fcvt_change_mode [(SI "df") (DI "sf")])
+
+(define_mode_attr FCVT_CHANGE_MODE [(SI "DF") (DI "SF")])
+
 ;; For scalar usage of vector/FP registers
 (define_mode_attr v [(QI "b") (HI "h") (SI "s") (DI "d")
 		    (HF  "h") (SF "s") (DF "d")
@@ -438,8 +445,8 @@
 (define_mode_attr rtn [(DI "d") (SI "")])
 (define_mode_attr vas [(DI "") (SI ".2s")])
 
-;; Map a floating point mode to the appropriate register name prefix
-(define_mode_attr s [(HF "h") (SF "s") (DF "d")])
+;; Map a floating point or integer mode to the appropriate register name prefix
+(define_mode_attr s [(HF "h") (SF "s") (DF "d") (SI "s") (DI "d")])
 
 ;; Give the length suffix letter for a sign- or zero-extension.
 (define_mode_attr size [(QI "b") (HI "h") (SI "w")])
diff --git a/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
new file mode 100644
index 0000000..0441458
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/fix_trunc1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+float
+f1 (float x)
+{
+  int y = x;
+
+  return (float) y;
+}
+
+double
+f2 (double x)
+{
+  long y = x;
+
+  return (double) y;
+}
+
+/* { dg-final { scan-assembler "fcvtzs\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\ts\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzs\\td\[0-9\]+, d\[0-9\]+" } } */
+/* { dg-final { scan-assembler "scvtf\\td\[0-9\]+, d\[0-9\]+" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c b/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c
index a1422d7..436399c 100644
--- a/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c
+++ b/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c
@@ -56,13 +56,13 @@ TEST (SUFFIX, q, 32, 4, u,u,s)			\
 TEST (SUFFIX, q, 64, 2, u,u,d)			\
 
 BUILD_VARIANTS ( )
-/* { dg-final { scan-assembler "fcvtzs\\tw\[0-9\]+, s\[0-9\]+" } } */
-/* { dg-final { scan-assembler "fcvtzs\\tx\[0-9\]+, d\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzs\\t(w|s)\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzs\\t(x|d)\[0-9\]+, d\[0-9\]+" } } */
 /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.2s, v\[0-9\]+\.2s" } } */
 /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s" } } */
 /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.2d, v\[0-9\]+\.2d" } } */
-/* { dg-final { scan-assembler "fcvtzu\\tw\[0-9\]+, s\[0-9\]+" } } */
-/* { dg-final { scan-assembler "fcvtzu\\tx\[0-9\]+, d\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzu\\t(w|s)\[0-9\]+, s\[0-9\]+" } } */
+/* { dg-final { scan-assembler "fcvtzu\\t(x|d)\[0-9\]+, d\[0-9\]+" } } */
 /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.2s, v\[0-9\]+\.2s" } } */
 /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s" } } */
 /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.2d, v\[0-9\]+\.2d" } } */
-- 
1.9.1

Re: [PING][PATCH][Aarch64] Improve int<->FP conversions

[James Greenhalgh]

On Tue, Oct 24, 2017 at 10:47:32PM +0100, Michael Collison wrote:
> James,
> 
> The patch was test as required. However when I tested with the latest trunk there were two test failures that need to be updated because of my patch. 
> 
> The files gcc.target/aarch64/vect-vcvt.c was failing because the scan-assembler directives assumed the destination register was assumed to be an integer register. With my patch the destination can be an integer or fp register.
> 
> I fixed the failures and bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?

OK.

> --- a/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c
> +++ b/gcc/testsuite/gcc.target/aarch64/vect-vcvt.c
> @@ -56,13 +56,13 @@ TEST (SUFFIX, q, 32, 4, u,u,s)			\
>  TEST (SUFFIX, q, 64, 2, u,u,d)			\
>  
>  BUILD_VARIANTS ( )
> -/* { dg-final { scan-assembler "fcvtzs\\tw\[0-9\]+, s\[0-9\]+" } } */
> -/* { dg-final { scan-assembler "fcvtzs\\tx\[0-9\]+, d\[0-9\]+" } } */
> +/* { dg-final { scan-assembler "fcvtzs\\t(w|s)\[0-9\]+, s\[0-9\]+" } } */
> +/* { dg-final { scan-assembler "fcvtzs\\t(x|d)\[0-9\]+, d\[0-9\]+" } } */
>  /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.2s, v\[0-9\]+\.2s" } } */
>  /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s" } } */
>  /* { dg-final { scan-assembler "fcvtzs\\tv\[0-9\]+\.2d, v\[0-9\]+\.2d" } } */
> -/* { dg-final { scan-assembler "fcvtzu\\tw\[0-9\]+, s\[0-9\]+" } } */
> -/* { dg-final { scan-assembler "fcvtzu\\tx\[0-9\]+, d\[0-9\]+" } } */
> +/* { dg-final { scan-assembler "fcvtzu\\t(w|s)\[0-9\]+, s\[0-9\]+" } } */
> +/* { dg-final { scan-assembler "fcvtzu\\t(x|d)\[0-9\]+, d\[0-9\]+" } } */
>  /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.2s, v\[0-9\]+\.2s" } } */
>  /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.4s, v\[0-9\]+\.4s" } } */
>  /* { dg-final { scan-assembler "fcvtzu\\tv\[0-9\]+\.2d, v\[0-9\]+\.2d" } } */

Personally I'd have used \[ws\] but this works too.

Reviewed by: James Greenhalgh <james.greenhalgh@arm.com>

James