Ping^4 Re: [Patch AArch64] Add floatdihf2 and floatunsdihf2 patterns

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Wed, Sep 28, 2016 at 05:17:14PM +0100, James Greenhalgh wrote:
> On Wed, Sep 21, 2016 at 10:42:03AM +0100, James Greenhalgh wrote:
> > On Tue, Sep 13, 2016 at 10:31:28AM +0100, James Greenhalgh wrote:
> > > On Tue, Sep 06, 2016 at 10:19:50AM +0100, James Greenhalgh wrote:
> > > > This patch adds patterns for conversion from 64-bit integer to 16-bit
> > > > floating-point values under AArch64 targets which don't have support for
> > > > the ARMv8.2-A 16-bit floating point extensions.
> > > > 
> > > > We implement these by first saturating to a SImode (we know that any
> > > > values >= 65504 will round to infinity after conversion to HFmode), then
> > > > converting to a DFmode (unsigned conversions could go to SFmode, but there
> > > > is no performance benefit to this). Then converting to HFmode.
> > > > 
> > > > Having added these patterns, the expansion path in "expand_float" will
> > > > now try to use them for conversions from SImode to HFmode as there is no
> > > > floatsihf2 pattern. expand_float first tries widening the integer size and
> > > > looking for a match, so it will try SImode -> DImode. But our DI mode
> > > > pattern is going to then saturate us back to SImode which is wasteful.
> > > > 
> > > > Better, would be for us to provide float(uns)sihf2 patterns directly.
> > > > So that's what this patch does.
> > > > 
> > > > The testcase add in this patch would fail on trunk for AArch64. There is
> > > > no libgcc routine to make the conversion, and we don't provide appropriate
> > > > patterns in the backend, so we get a link-time error.
> > > > 
> > > > Bootstrapped and tested on aarch64-none-linux-gnu
> > > > 
> > > > OK for trunk?
> > > 
> > > Ping.
> > 
> > Ping^2
> 
> Ping^3

Ping^4

Thanks,
James

> 
> There was an off-list question as to whether the mid-end could catch this,
> rather than requiring the target to do so. My objection to that is that it
> would involve teaching the midend about saturating narrowing operations,
> which if the target doesn't provide them natively require branching.
> 
> I'd rather push targets that want DImode to HFmode (and don't provide a
> DImode to TFmode to go through first) to use libgcc/soft-fp than try to add
> a special generic expander for DImode to HFmode conversions.
> 
> Note that even if we did have a generic expander for these types, we would
> still need some version of this patch, as we want to override the behaviour
> where the ARMv8.2-A 16-bit floating-point types are available.
> > > > 2016-09-06  James Greenhalgh  <james.greenhalgh@arm.com>
> > > > 
> > > > 	* config/aarch64/aarch64.md (<optab>sihf2): Convert to expand.
> > > > 	(<optab>dihf2): Likewise.
> > > > 	(aarch64_fp16_<optab><mode>hf2): New.
> > > > 
> > > > 2016-09-06  James Greenhalgh  <james.greenhalgh@arm.com>
> > > > 
> > > > 	* gcc.target/aarch64/floatdihf2_1.c: New.
> > > > 
> > > 
> > > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> > > > index 6afaf90..1882a72 100644
> > > > --- a/gcc/config/aarch64/aarch64.md
> > > > +++ b/gcc/config/aarch64/aarch64.md
> > > > @@ -4630,7 +4630,14 @@
> > > >    [(set_attr "type" "f_cvti2f")]
> > > >  )
> > > >  
> > > > -(define_insn "<optab><mode>hf2"
> > > > +;; If we do not have ARMv8.2-A 16-bit floating point extensions, the
> > > > +;; midend will arrange for an SImode conversion to HFmode to first go
> > > > +;; through DFmode, then to HFmode.  But first it will try converting
> > > > +;; to DImode then down, which would match our DImode pattern below and
> > > > +;; give very poor code-generation.  So, we must provide our own emulation
> > > > +;; of the mid-end logic.
> > > > +
> > > > +(define_insn "aarch64_fp16_<optab><mode>hf2"
> > > >    [(set (match_operand:HF 0 "register_operand" "=w")
> > > >  	(FLOATUORS:HF (match_operand:GPI 1 "register_operand" "r")))]
> > > >    "TARGET_FP_F16INST"
> > > > @@ -4638,6 +4645,53 @@
> > > >    [(set_attr "type" "f_cvti2f")]
> > > >  )
> > > >  
> > > > +(define_expand "<optab>sihf2"
> > > > +  [(set (match_operand:HF 0 "register_operand")
> > > > +	(FLOATUORS:HF (match_operand:SI 1 "register_operand")))]
> > > > +  "TARGET_FLOAT"
> > > > +{
> > > > +  if (TARGET_FP_F16INST)
> > > > +    emit_insn (gen_aarch64_fp16_<optab>sihf2 (operands[0], operands[1]));
> > > > +  else
> > > > +    {
> > > > +      rtx convert_target = gen_reg_rtx (DFmode);
> > > > +      emit_insn (gen_<optab>sidf2 (convert_target, operands[1]));
> > > > +      emit_insn (gen_truncdfhf2 (operands[0], convert_target));
> > > > +    }
> > > > +  DONE;
> > > > +}
> > > > +)
> > > > +
> > > > +;; For DImode there is no wide enough floating-point mode that we
> > > > +;; can convert through natively (TFmode would work, but requires a library
> > > > +;; call).  However, we know that any value >= 65504 will be rounded
> > > > +;; to infinity on conversion.  This is well within the range of SImode, so
> > > > +;; we can:
> > > > +;;   Saturate to SImode.
> > > > +;;   Convert from that to DFmode
> > > > +;;   Convert from that to HFmode (phew!).
> > > > +;; Note that the saturation to SImode requires the SIMD extensions.  If
> > > > +;; we ever need to provide this pattern where the SIMD extensions are not
> > > > +;; available, we would need a different approach.
> > > > +
> > > > +(define_expand "<optab>dihf2"
> > > > +  [(set (match_operand:HF 0 "register_operand")
> > > > +	(FLOATUORS:HF (match_operand:DI 1 "register_operand")))]
> > > > +  "TARGET_FLOAT && (TARGET_FP_F16INST || TARGET_SIMD)"
> > > > +{
> > > > +  if (TARGET_FP_F16INST)
> > > > +    emit_insn (gen_aarch64_fp16_<optab>dihf2 (operands[0], operands[1]));
> > > > +  else
> > > > +    {
> > > > +      rtx sat_target = gen_reg_rtx (SImode);
> > > > +      emit_insn (gen_aarch64_<su_optab>qmovndi (sat_target, operands[1]));
> > > > +      emit_insn (gen_<optab>sihf2 (operands[0], sat_target));
> > > > +    }
> > > > +
> > > > +  DONE;
> > > > +}
> > > > +)
> > > > +
> > > >  ;; Convert between fixed-point and floating-point (scalar modes)
> > > >  
> > > >  (define_insn "<FCVT_F2FIXED:fcvt_fixed_insn><GPF:mode>3"
> > > > diff --git a/gcc/testsuite/gcc.target/aarch64/floatdihf2_1.c b/gcc/testsuite/gcc.target/aarch64/floatdihf2_1.c
> > > > new file mode 100644
> > > > index 0000000..9eaa4ba
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/aarch64/floatdihf2_1.c
> > > > @@ -0,0 +1,35 @@
> > > > +/* { dg-do compile } */
> > > > +/* { dg-options "-O2" } */
> > > > +
> > > > +/* Test that conversion from 32-bit and 64-bit integers can be done
> > > > +   without a call to the support library.  */
> > > > +
> > > > +#pragma GCC target ("arch=armv8.2-a+nofp16")
> > > > +
> > > > +__fp16
> > > > +foo (int x)
> > > > +{
> > > > +  return x;
> > > > +}
> > > > +
> > > > +__fp16
> > > > +bar (unsigned int x)
> > > > +{
> > > > +  return x;
> > > > +}
> > > > +
> > > > +__fp16
> > > > +fool (long long x)
> > > > +{
> > > > +  return x;
> > > > +}
> > > > +
> > > > +__fp16
> > > > +barl (unsigned long long x)
> > > > +{
> > > > +  return x;
> > > > +}
> > > > +
> > > > +
> > > > +/* { dg-final { scan-assembler-not "__float\\\[ds\\\]ihf2" } } */
> > > > +/* { dg-final { scan-assembler-not "__floatun\\\[ds\\\]ihf2" } } */
> > > 
> > 
>