Patch updated with all comments from James. Bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?
2017-10-15 Michael Collison <michael.colli...@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.greenha...@arm.com]
Sent: Wednesday, October 4, 2017 2:39 AM
To: Michael Collison <michael.colli...@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@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
pr6527v4.patch
Description: pr6527v4.patch
