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

[Jan Beulich <jbeulich@suse.com>]

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.

> * 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.

Jan