Re: [PATCH, ARM] ISO/IEC TR 18037 fixed-point support

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 11 May 2011, Julian Brown wrote:

> non-saturating variants where appropriate). Other functionality is
> provided by support functions in libgcc (type conversions,
> comparisons, and so on). Although more efficient versions of some of
> these could be implemented inline, it seems to me they would mostly use
> regular integer operations: I'm not at all sure that building that into
> each target rather than teaching generic code how to do it is a
> sensible way to go.

Indeed, the original fixed-point code seems rather badly thought out in 
that way.  Ideally you wouldn't have special machine modes at all; you'd 
have extra operations on ordinary modes, and some GIMPLE pass, or expand 
at the latest, would lower operations on fixed-point types into operations 
on ordinary types.  (You'd want appropriate representations in GIMPLE for 
e.g. saturating operations, see PR 48580; in addition to basic saturating 
operations, multiplication on fixed-point types corresponds to 
multiply/shift on generic types.)

I think that would do a better job of making instruction set features 
available to C code - ordinary C code using existing C idioms for 
saturation or overflow checks in some cases, rather than just code using 
these special extensions - where appropriate descriptions have been added 
to the target.  But it's certainly a lot more work.

> There's currently no support for "long long" fixed-point types, since
> those imply support for TImode arithmetic, which isn't sensible on ARM.
> I've modified tests to make optional the requirement for long long
> support.

I wonder how those tests work, or don't work, on 32-bit MIPS, which ought 
to have the same issue....

>     * optabs.c (prepare_cmp_insn): Use correct biasing for fixed-point
>     comparison helpers.
>     * final.c (output_addr_const): Print fixed-point constants as
>     decimal not hex (avoiding an assembler overflow warning for
>     negative byte constants).
>     * tree-ssa-sccvn.c (copy_reference_ops_from_ref): Support FIXED_CST.

>     * config/fixed-bit.h (fixed-bit-machine.h): Include.
>     (DECORATE_FIXED_NAME): Conditionally define if undefined. Use
>     throughout file to decorate function names.
>     * config/fixed-bit-mach-generic.h: New.

It might make sense to send the machine-independent parts separately, 
under a separate subject heading, with an explanation for why each bit is 
needed.

>     libgcc/
>     * config.host (arm*-*-linux*, arm*-*-uclinux*, arm*-*-eabi*)
>     (arm*-*-nucleuseabi*, arm*-*-symbianelf*): Add arm/t-fixed-point

The arm*-*-nucleuseabi* reference seems bogus.

> +/* Return TRUE if fixed-point operations are supported.  */
> +
> +bool
> +arm_fixed_point_supported_p (void)
> +{
> +  /* At least some operations helpful for fixed-point support are available
> +     from arch3m onwards -- 32 bit x 32 bit -> 64 bit multiplies.  There's not
> +     much point in arbitrarily restricting fixed-point support to particular
> +     architecture versions though since gaps can be filled by libcalls, so
> +     return TRUE unconditionally.  */
> +  return true;
> +}

Is there a reason for having a separate function, rather than just using 
hook_bool_void_true (with this comment going on the definition of 
TARGET_FIXED_POINT_SUPPORTED_P)?

I don't see any change to the code in gcc/configure.ac that handles 
--enable-fixed-point to know that ARM targets now support it.  Since the 
default hook returns ENABLE_FIXED_POINT, maybe if you fixed the configure 
test you wouldn't need to define the hook for ARM at all....

> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 40ebf35..8a1dd76 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -10751,3 +10751,5 @@
>  (include "neon.md")
>  ;; Synchronization Primitives
>  (include "sync.md")
> +;; Fixed-point patterns
> +(include "arm-fixed.md")

Should MD_INCLUDES in t-arm be updated?

> diff --git a/gcc/config/arm/fixed-bit-machine.h b/gcc/config/arm/fixed-bit-machine.h
> new file mode 100644
> index 0000000..c7ce8ec
> --- /dev/null
> +++ b/gcc/config/arm/fixed-bit-machine.h
> @@ -0,0 +1,29 @@
> +/* Machine-specific override for fixed-point support routine names.
> +   Copyright (C) 2011 Free Software Foundation, Inc.
> +
> +   This file is part of GCC.
> +
> +   GCC is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published
> +   by the Free Software Foundation; either version 3, or (at your
> +   option) any later version.
> +
> +   GCC is distributed in the hope that it will be useful, but WITHOUT
> +   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +   or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> +   License for more details.
> +
> +   Under Section 7 of GPL version 3, you are granted additional
> +   permissions described in the GCC Runtime Library Exception, version
> +   3.1, as published by the Free Software Foundation.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */

Wrong version of the last paragraph.  See e.g. libgcc2.c.  "GNU General 
Public License and a copy of the GCC Runtime Library Exception ... 
COPYING3 and COPYING.RUNTIME respectively".

> diff --git a/gcc/config/fixed-bit-mach-generic.h b/gcc/config/fixed-bit-mach-generic.h
> new file mode 100644
> index 0000000..c25f0a5
> --- /dev/null
> +++ b/gcc/config/fixed-bit-mach-generic.h
> @@ -0,0 +1,22 @@
> +/* Null machine-specific override for fixed-point support routine names.
> +   Copyright (C) 2011 Free Software Foundation, Inc.
> +
> +   This file is part of GCC.
> +
> +   GCC is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published
> +   by the Free Software Foundation; either version 3, or (at your
> +   option) any later version.
> +
> +   GCC is distributed in the hope that it will be useful, but WITHOUT
> +   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +   or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> +   License for more details.
> +
> +   Under Section 7 of GPL version 3, you are granted additional
> +   permissions described in the GCC Runtime Library Exception, version
> +   3.1, as published by the Free Software Foundation.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */

Likewise.

-- 
Joseph S. Myers
joseph@codesourcery.com