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 6:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 30.03.2023 17:36, Christoph Müllner wrote:
> > 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.
>
> Why parsing two strings? The table values could all be pre-computed.

Agree.

>
> > * 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).
>
> Fair enough I guess as long as it's properly spelled out somewhere,
> so that people running into issues don't need to dig through half
> the assembler.

I just noticed that gas/doc/c-riscv.texi is missing a RISC-V
Floating-point section.
A new revision of this patch will include the information in there
(including the error
message that is expected on such a machine).

Besides that I will also replace scanf() by strtof() in the new revision.