Re: [PATCH 8/17][ARM] Add VFP FP16 arithmetic instructions.

[James Greenhalgh <james.greenhalgh@arm.com>]

On Wed, Aug 03, 2016 at 12:52:42PM +0100, Ramana Radhakrishnan wrote:
> On Thu, Jul 28, 2016 at 12:37 PM, Ramana Radhakrishnan
> <ramana.gcc@googlemail.com> wrote:
> > On Mon, Jul 4, 2016 at 3:02 PM, Matthew Wahab
> > <matthew.wahab@foss.arm.com> wrote:
> >> On 19/05/16 15:54, Matthew Wahab wrote:
> >>> On 18/05/16 16:20, Joseph Myers wrote:
> >>>> On Wed, 18 May 2016, Matthew Wahab wrote:
> >>>>
> >>>> In short: instructions for direct HFmode arithmetic should be described
> >>>> with patterns with the standard names.  It's the job of the
> >>>> architecture-independent compiler to ensure that fp16 arithmetic in the
> >>>> user's source code only generates direct fp16 arithmetic in GIMPLE (and
> >>>> thus ends up using those patterns) if that is a correct representation of
> >>>> the source code's semantics according to ACLE.
> >>>>
> >>>> The intrinsics you provide can then be written to use direct arithmetic,
> >>>> and rely on convert_to_real_1 eliminating the promotions, rather than
> >>>> needing built-in functions at all, just like many arm_neon.h intrinsics
> >>>> make direct use of GNU C vector arithmetic.
> >>>
> >>> I think it's clear that this has exhausted my knowledge of FP semantics.
> >>>
> >>> Forcing promotion to single-precision was to settle concerns brought up in
> >>> internal discussions about __fp16 semantics. I'll see if anybody has any
> >>> problem with the changes you suggest.
> >>
> >> This patch changes the implementation to use the standard names for the
> >> HFmode arithmetic. Later patches will also be updated to use the
> >> arithmetic operators where appropriate.
> >>
> >> Changes since the last version of this patch:
> >> - The standard names for plus, minus, mult, div and fma are defined for
> >>   HF mode.
> >> - The patterns supporting the new ACLE intrinsics vnegh_f16, vaddh_f16,
> >>   vsubh_f16, vmulh_f16 and vdivh_f16 are removed, the arithmetic
> >>   operators will be used instead.
> >> - The tests are updated to expect f16 instructions rather than the f32
> >>   instructions that were previously emitted.
> >>
> >> Tested the series for arm-none-linux-gnueabihf with native bootstrap and
> >> make check and for arm-none-eabi and armeb-none-eabi with make check on
> >> an ARMv8.2-A emulator.
> >
> >
> > All fine except -
> >
> > Why can we not extend the <vrint_pattern> and the l<vrint_pattern> in
> > vfp.md for fp16 and avoid all the unspecs for vcvta and vrnd*
> > instructions ?
> >
> 
> I now feel reasonably convinced that these can go away and be replaced
> by extending the <vrint_pattern> and l<vrint_pattern> expanders to
> consider FP16 as well. Given that we are still only in the middle of
> stage1 - I'm ok for you to apply this as is and then follow-up with a
> patch that gets rid of the UNSPECs . If this holds for add, sub and
> other patterns I don't see why it wouldn't hold for all these patterns
> as well.
> 
> Joseph, do you have any opinions on whether we should be extending the
> standard pattern names or not for btrunc, ceil, round, floor,
> nearbyint, rint, lround, lfloor and lceil optabs for the HFmode
> quantities ?

Mapping these to standard pattern names is the right thing to do if they
implement the correct semantics for those standard pattern names. That's
true whether you access them by function name (as you would for _Float16),
or as intrinsics (as you may want to do for __fp16 in arm_fp16.h).

I see that the ARM port doesn't have as general a mechanism for specifying
intrinsics in config/arm/arm_neon_builtins.def as the AArch64 port has in
config/aarch64/aarch64-simd-builtins.def . In the AArch64 port it is
perfectly acceptable for a builtin to map on to a standard pattern name.
In the ARM port it seems there is a limitation such that all builtins *must*
map on to pattern names with the prefix "neon_".

Fixing this limitation (perhaps in the way that AArch64 goes about it with
a series of magic macros) would permit these to be Standard Pattern names.
See https://gcc.gnu.org/ml/gcc-patches/2013-04/msg01219.html for what I did
to AArch64 3 years ago.

I think that's probably the right way to go about resolving this, but I
haven't looked too hard in to what it would take in the ARM port to refactor
along those lines.
 
Thanks,
James