Re: [PATCH v8] RISC-V: Add the 'zfa' extension, version 0.2.

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/19/23 03:57, Jin Ma wrote:
> This patch adds the 'Zfa' extension for riscv, which is based on:
>    https://github.com/riscv/riscv-isa-manual/commits/zfb
>    https://github.com/riscv/riscv-isa-manual/commit/1f038182810727f5feca311072e630d6baac51da
> 
> The binutils-gdb for 'Zfa' extension:
>    https://github.com/a4lg/binutils-gdb/commits/riscv-zfa
> 
> What needs special explanation is:
> 1, The immediate number of the instructions FLI.H/S/D is represented in the assembly as a
>    floating-point value, with scientific counting when rs1 is 1,2, and decimal numbers for
>    the rest.
> 
>    Related llvm link:
>      https://reviews.llvm.org/D145645
>    Related discussion link:
>      https://github.com/riscv/riscv-isa-manual/issues/980
Right.  I think the goal right now is to get the bulk of this reviewed 
now.  Ideally we'll get to the point where the only outstanding issue is 
the interface between the assembler & gcc.

> 
> 2, According to riscv-spec, "The FCVTMO D.W.D instruction was added principally to
>    accelerate the processing of JavaScript Numbers.", so it seems that no implementation
>    is required.
Fair enough.  There's seems to be a general desire to wire up builtins 
for many things that aren't directly usable by the compiler.  So 
consider such a change as a follow-up.   I don't think something like 
this should hold up the blk of Zfa.

> 
> 3, The instructions FMINM and FMAXM correspond to C23 library function fminimum and fmaximum.
>    Therefore, this patch has simply implemented the pattern of fminm<hf\sf\df>3 and
>    fmaxm<hf\sf\df>3 to prepare for later.
Sounds good.


> 
> gcc/ChangeLog:
> 
> 	* common/config/riscv/riscv-common.cc: Add zfa extension version.
> 	* config/riscv/constraints.md (Zf): Constrain the floating point number that the
> 	instructions FLI.H/S/D can load.
> 	((TARGET_XTHEADFMV || TARGET_ZFA) ? FP_REGS : NO_REGS): enable FMVP.D.X and FMVH.X.D.
> 	* config/riscv/iterators.md (ceil): New.
> 	* config/riscv/riscv-protos.h (riscv_float_const_rtx_index_for_fli): New.
> 	* config/riscv/riscv.cc (find_index_in_array): New.
> 	(riscv_float_const_rtx_index_for_fli): Get the index of the floating-point number that
> 	the instructions FLI.H/S/D can mov.
> 	(riscv_cannot_force_const_mem): If instruction FLI.H/S/D can be used, memory is not applicable.
> 	(riscv_const_insns): The cost of FLI.H/S/D is 3.
> 	(riscv_legitimize_const_move): Likewise.
> 	(riscv_split_64bit_move_p): If instruction FLI.H/S/D can be used, no split is required.
> 	(riscv_output_move): Output the mov instructions in zfa extension.
> 	(riscv_print_operand): Output the floating-point value of the FLI.H/S/D immediate in assembly
> 	(riscv_secondary_memory_needed): Likewise.
> 	* config/riscv/riscv.h (GP_REG_RTX_P): New.
> 	* config/riscv/riscv.md (fminm<mode>3): New.
> 

> index c448e6b37e9..62d9094f966 100644
> --- a/gcc/config/riscv/constraints.md
> +++ b/gcc/config/riscv/constraints.md
> @@ -118,6 +118,13 @@ (define_constraint "T"
>     (and (match_operand 0 "move_operand")
>          (match_test "CONSTANT_P (op)")))
>   
> +;; Zfa constraints.
> +
> +(define_constraint "Zf"
> +  "A floating point number that can be loaded using instruction `fli` in zfa."
> +  (and (match_code "const_double")
> +       (match_test "(riscv_float_const_rtx_index_for_fli (op) != -1)")))
> +
>   ;; Vector constraints.
>   
>   (define_register_constraint "vr" "TARGET_VECTOR ? V_REGS : NO_REGS"
> @@ -183,8 +190,8 @@ (define_memory_constraint "Wdm"
>   
>   ;; Vendor ISA extension constraints.
>   
> -(define_register_constraint "th_f_fmv" "TARGET_XTHEADFMV ? FP_REGS : NO_REGS"
> +(define_register_constraint "th_f_fmv" "(TARGET_XTHEADFMV || TARGET_ZFA) ? FP_REGS : NO_REGS"
>     "A floating-point register for XTheadFmv.")
>   
> -(define_register_constraint "th_r_fmv" "TARGET_XTHEADFMV ? GR_REGS : NO_REGS"
> +(define_register_constraint "th_r_fmv" "(TARGET_XTHEADFMV || TARGET_ZFA) ? GR_REGS : NO_REGS"
>     "An integer register for XTheadFmv.")
I think Christoph had good suggestions on the constraints.  So let's go 
with his suggestions.

You might consider a follow-up patch where you use negation of one of 
the predefined constants for synthesis.  I would not be surprised at all 
if that's as efficient on some cores as loading the negated constants 
out of the constant pool.  But I don't think it has to be a part of this 
patch.




> diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md
> index 9b767038452..c81b08e3cc5 100644
> --- a/gcc/config/riscv/iterators.md
> +++ b/gcc/config/riscv/iterators.md
> @@ -288,3 +288,8 @@ (define_int_iterator QUIET_COMPARISON [UNSPEC_FLT_QUIET UNSPEC_FLE_QUIET])
>   (define_int_attr quiet_pattern [(UNSPEC_FLT_QUIET "lt") (UNSPEC_FLE_QUIET "le")])
>   (define_int_attr QUIET_PATTERN [(UNSPEC_FLT_QUIET "LT") (UNSPEC_FLE_QUIET "LE")])
>   
> +(define_int_iterator ROUND [UNSPEC_ROUND UNSPEC_FLOOR UNSPEC_CEIL UNSPEC_BTRUNC UNSPEC_ROUNDEVEN UNSPEC_NEARBYINT])
> +(define_int_attr round_pattern [(UNSPEC_ROUND "round") (UNSPEC_FLOOR "floor") (UNSPEC_CEIL "ceil")
> +				(UNSPEC_BTRUNC "btrunc") (UNSPEC_ROUNDEVEN "roundeven") (UNSPEC_NEARBYINT "nearbyint")])
> +(define_int_attr round_rm [(UNSPEC_ROUND "rmm") (UNSPEC_FLOOR "rdn") (UNSPEC_CEIL "rup")
> +			   (UNSPEC_BTRUNC "rtz") (UNSPEC_ROUNDEVEN "rne") (UNSPEC_NEARBYINT "dyn")])
Do we really need to use unspecs for all these cases?  I would expect 
some correspond to the trunc, round, ceil, nearbyint, etc well known RTX 
codes.

In general, we should try to avoid unspecs when there is a clear 
semantic match between the instruction and GCC's RTX opcodes.  So please 
review the existing RTX code semantics to see if any match the new 
instructions.  If there are matches, use those RTX codes rather than 
UNSPECs.



>   
> +/* Immediate values loaded by the FLI.S instruction in Chapter 25 of the latest RISC-V ISA
> +   Manual draft. For details, please see:
> +   https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20221217-cb3b9d1 */
> +
> +unsigned HOST_WIDE_INT fli_value_hf[32] =
> +{
> +  0xbc00, 0x400, 0x100, 0x200, 0x1c00, 0x2000, 0x2c00, 0x3000,
> +  0x3400, 0x3500, 0x3600, 0x3700, 0x3800, 0x3900, 0x3a00, 0x3b00,
> +  0x3c00, 0x3d00, 0x3e00, 0x3f00, 0x4000, 0x4100, 0x4200, 0x4400,
> +  0x4800, 0x4c00, 0x5800, 0x5c00, 0x7800,
> +  /* Only used for filling, ensuring that 29 and 30 of HF are the same. */
> +  0x7800,
> +  0x7c00, 0x7e00,
> +};
> +
> +unsigned HOST_WIDE_INT fli_value_sf[32] =
> +{
> +  0xbf800000, 0x00800000, 0x37800000, 0x38000000, 0x3b800000, 0x3c000000, 0x3d800000, 0x3e000000,
> +  0x3e800000, 0x3ea00000, 0x3ec00000, 0x3ee00000, 0x3f000000, 0x3f200000, 0x3f400000, 0x3f600000,
> +  0x3f800000, 0x3fa00000, 0x3fc00000, 0x3fe00000, 0x40000000, 0x40200000, 0x40400000, 0x40800000,
> +  0x41000000, 0x41800000, 0x43000000, 0x43800000, 0x47000000, 0x47800000, 0x7f800000, 0x7fc00000
> +};
> +
> +unsigned HOST_WIDE_INT fli_value_df[32] =
> +{
> +  0xbff0000000000000, 0x10000000000000, 0x3ef0000000000000, 0x3f00000000000000,
> +  0x3f70000000000000, 0x3f80000000000000, 0x3fb0000000000000, 0x3fc0000000000000,
> +  0x3fd0000000000000, 0x3fd4000000000000, 0x3fd8000000000000, 0x3fdc000000000000,
> +  0x3fe0000000000000, 0x3fe4000000000000, 0x3fe8000000000000, 0x3fec000000000000,
> +  0x3ff0000000000000, 0x3ff4000000000000, 0x3ff8000000000000, 0x3ffc000000000000,
> +  0x4000000000000000, 0x4004000000000000, 0x4008000000000000, 0x4010000000000000,
> +  0x4020000000000000, 0x4030000000000000, 0x4060000000000000, 0x4070000000000000,
> +  0x40e0000000000000, 0x40f0000000000000, 0x7ff0000000000000, 0x7ff8000000000000,
> +};
Going to assume these are sane.  I think the only concern would be 
endianness, but it looks like you handle that reasonably.
> +
> +/* Find the index of TARGET in ARRAY, and return -1 if not found. */
> +
> +static int
> +find_index_in_array (unsigned HOST_WIDE_INT target, unsigned HOST_WIDE_INT *array, int len)
> +{
> +  if (array == NULL)
> +    return -1;
> +
> +  for (int i = 0; i < len; i++)
> +    {
> +      if (target == array[i])
> +	return i;
> +    }
> +  return -1;
> +}
Given the way constraint and operand matching occurrs, I wouldn't be 
surprised if this search turns out to be compile-time expensive.



> +
> +/* Return index of the FLI instruction table if rtx X is an immediate constant that
> +   can be moved using a single FLI instruction in zfa extension. -1 otherwise. */
> +
> +int
> +riscv_float_const_rtx_index_for_fli (rtx x)
> +{
> +  machine_mode mode = GET_MODE (x);
> +
> +  if (!TARGET_ZFA || mode == VOIDmode
> +      || !CONST_DOUBLE_P(x)
> +      || (mode == HFmode && !TARGET_ZFH)
> +      || (mode == SFmode && !TARGET_HARD_FLOAT)
> +      || (mode == DFmode && !TARGET_DOUBLE_FLOAT))
> +    return -1;
Bring the "|| mode == VOIDmode" down to its own line similar to how 
you've done with the !CONST_DOUBLE_P check.



>   
>   static bool
> @@ -826,6 +936,9 @@ riscv_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>     if (GET_CODE (x) == HIGH)
>       return true;
>   
> +  if (riscv_float_const_rtx_index_for_fli (x) != -1)
> +   return true;
> +
So if you do a follow-up handling negative fli constants, obviously we'd 
need further changes in this code.


> @@ -1213,6 +1326,11 @@ riscv_const_insns (rtx x)
>         }
>   
>       case CONST_DOUBLE:
> +      /* See if we can use FMV directly.  */
> +      if (riscv_float_const_rtx_index_for_fli (x) != -1)
> +	return 3;
That seems fairly high cost-wise.  Where did this value come from?   Or 
is it relative to COSTS_N_INSNS?




>     if (TARGET_DOUBLE_FLOAT
>         && ((FP_REG_RTX_P (src) && FP_REG_RTX_P (dest))
>   	  || (FP_REG_RTX_P (dest) && MEM_P (src))
>   	  || (FP_REG_RTX_P (src) && MEM_P (dest))
> +	  || (TARGET_ZFA
> +	      && ((FP_REG_RTX_P (dest) && GP_REG_RTX_P (src))
> +	      || (FP_REG_RTX_P (src) && GP_REG_RTX_P (dest))))
The formatting of the second FP_REG_RTX_P check looks goofy, but that 
may be a mailer issue.  Double check the "|| FP_REG" should line up 
under the FP_REG_RTX_P.




> @@ -2968,6 +3103,14 @@ riscv_output_move (rtx dest, rtx src)
>   	  case 8:
>   	    return "fld\t%0,%1";
>   	  }
> +
> +      if (src_code == CONST_DOUBLE && (riscv_float_const_rtx_index_for_fli (src) != -1))
> +	switch (width)
> +	  {
> +	    case 2: return "fli.h\t%0,%1";
> +	    case 4: return "fli.s\t%0,%1";
> +	    case 8: return "fli.d\t%0,%1";
> +	  }
We generally discourage having code on the same line as a case 
statement, so bring those return statements down to a new line.





> @@ -1580,6 +1609,26 @@ (define_insn "l<rint_pattern><ANYF:mode><GPR:mode>2"
>     [(set_attr "type" "fcvt")
>      (set_attr "mode" "<ANYF:MODE>")])
>   
> +(define_insn "<round_pattern><ANYF:mode>2"
> +  [(set (match_operand:ANYF     0 "register_operand" "=f")
> +	(unspec:ANYF
> +	    [(match_operand:ANYF 1 "register_operand" " f")]
> +	ROUND))]
> +  "TARGET_HARD_FLOAT && TARGET_ZFA"
> +  "fround.<ANYF:fmt>\t%0,%1,<round_rm>"
> +  [(set_attr "type" "fcvt")
> +   (set_attr "mode" "<ANYF:MODE>")])
> +
> +(define_insn "rint<ANYF:mode>2"
> +  [(set (match_operand:ANYF     0 "register_operand" "=f")
> +	(unspec:ANYF
> +	    [(match_operand:ANYF 1 "register_operand" " f")]
> +	UNSPEC_RINT))]
> +  "TARGET_HARD_FLOAT && TARGET_ZFA"
> +  "froundnx.<ANYF:fmt>\t%0,%1"
> +  [(set_attr "type" "fcvt")
> +   (set_attr "mode" "<ANYF:MODE>")])
Please review the existing RTX codes and their semantics in the 
internals manual and if any of the new instructions match those existing 
primitives, implement them using those RTX codes rather than with an UNSPEC.


Overall it looks pretty good.

jeff