Re: [RFC PATCH v2 2/2] RISC-V: Add support for the Zfa extension

["Christoph Müllner" <christoph.muellner@vrull.eu>]

On Thu, Mar 30, 2023 at 2:18 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 30.03.2023 12:54, Jan Beulich via Binutils wrote:
> > On 30.03.2023 12:30, Christoph Müllner wrote:
> >> On Mon, Mar 27, 2023 at 11:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 27.03.2023 10:53, Kito Cheng wrote:
> >>>> Wait, I mean the hex floating point format defined in C99/C++17, not
> >>>> the raw hex value.
> >>>> so something like 0x1p-16 (0.0000152587890625), 0x1p-2 (0.25) 0x1p+0,
> >>>> -0x1p+0 could be used for fli.* instruction.
> >>>>
> >>>> You could use printf with %a to get those values.
> >>>>
> >>>> https://gcc.gnu.org/onlinedocs/gcc/Hex-Floats.html
> >>>> https://developer.arm.com/documentation/dui0375/latest/Compiler-Coding-Practices/Hexadecimal-floating-point-numbers-in-C99
> >>>
> >>> Sure, my (secondary) suggestion ...
> >>>
> >>>> On Mon, Mar 27, 2023 at 4:39 PM Jan Beulich via Binutils
> >>>> <binutils@sourceware.org> wrote:
> >>>>>
> >>>>> On 27.03.2023 10:01, Christoph Muellner wrote:
> >>>>>> --- a/opcodes/riscv-opc.c
> >>>>>> +++ b/opcodes/riscv-opc.c
> >>>>>> @@ -110,6 +110,16 @@ const char * const riscv_vma[2] =
> >>>>>>    "mu", "ma"
> >>>>>>  };
> >>>>>>
> >>>>>> +/* The FLI.[HSDQ] value constants.  */
> >>>>>> +const char * const riscv_fli_value[32] =
> >>>>>> +{
> >>>>>> +  "-1.0", "min", "0.0000152587890625", "0.000030517578125",
> >>>>>> +  "0.00390625", "0.0078125", "0.0625", "0.125",
> >>>>>> +  "0.25", "0.3125", "0.375", "0.4375", "0.5", "0.625", "0.75", "0.875",
> >>>>>> +  "1.0", "1.25", "1.5", "1.75", "2.0", "2.5", "3.0", "4.0",
> >>>>>> +  "8.0", "16.0", "128.0", "256.0", "32768.0", "65536.0",  "inf", "nan",
> >>>>>> +};
> >>>>>
> >>>>> Especially for values like 1.0x2^^-n (entries 2 and onwards) I question
> >>>>> the spelled out numbers to be the most suitable ones usability wise. At
> >>>>> least some alternative spelling (e.g. 2.e-16) ought to be recognized as
> >>>>> well. But since there are meany reasonable spellings (leading 0 omitted
> >>>>> in 0.<fraction> or trailing zero omitted in <num>.0), I guess I'd prefer
> >>>>> if values were actually parsed as a floating point number (e.g. via
> >>>>> ieee_md_atof()), and then matched against values stored in the table.
> >>>>> One might further consider to also permit the 2nd form accepted
> >>>>> elsewhere, see read.c:parse_one_float().
> >>>
> >>> ... here wasn't meant to collide with yours. What you're asking for is
> >>> covered by my primary suggestion (to actually parse the values), extended
> >>> by the need to actually recognize C99 hex float in the parser then (leaving
> >>> aside for now whether that's feasible in the first place).
> >>
> >> Thanks for all the suggestions!
> >>
> >> I worked my way through this and I believe that the following would be
> >> a reasonable solution:
> >> * constants min, inf and nan must be symbols (as stated in the specification)
> >> * other constants are parsed by scanf("%f") as floats and compared
> >> (float compare in C) against the numeric constants in the table
> >> * output in the disassembly uses symbols for min/inf/nan and %a (hex
> >> FP literals) for other constants
> >>
> >> So we support every format that '%f' accepts including hex FP literals
> >> (e.g. -0x1p0, 0x1p+0, ...) and normal FP constants (e.g.
> >> 0.0000152587890625, 25E-4).
> >
> > How's this going to work with a cross-assembler run on an architecture
> > supporting a floating point format other than IEEE 754's? Hence why I
> > suggested using ieee_md_atof() instead.
>
> Hmm, I see riscv_fli_numval[] has "float" as the base type (which I
> didn't really expect), so this ought to work as long as the initializers
> used can be represented exactly in whatever arch's floating point format.

The idea to compare parsed FP numbers was yours (thanks for that!).
To do that the use of floats seemed obvious as I did not see why using the
atof-ieee machinery was necessary (I did not consider non IEEE 754 machines).

Since we only parse and compare, at least the hex FP literals should
work on all machines (otherwise these machines would have more serious issues).
As I don't have access to non IEEE 754 machines I can not test other
representations.

I now had a look at the atof-ieee framework and found the following issues:
* ieee_md_atof() does not reveal the number of parsed bytes
  Workaround: use atof_ieee() directly instead (access to F_PRECISION would
  be nice to not have to compare the whole LITTLENUM_TYPE values).
* Parsing hex FP literals does not work
  Input: 0x1p-15
  Error: Could not parse input: x1p-15
* Parsing two strings will always be slower than parsing only one of them.
* Parsers in the atof-ieee framework don't care about pointer-to-const,
  but the constant tables are of type "const char * const".

I now understand that the solution in this patch is not perfect.
But given the alternative is not perfect either, I would prefer to
keep the approach
of this patch unless we identify that it is really broken on an
existing machine that
does not use IEEE 754 FP (we can add more test cases to spot that
during testing).