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

[Michael Collison <Michael dot Collison at arm dot com>]

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: pr6527v4.patch
Description: pr6527v4.patch