Repository: bytecodealliance/wasmtime
Pull request #8536 (2024-05-03 10:47:40)

riscv64: Add minimal support for the Zfa Extension
👋 Hey,

This PR adds initial support for the Zfa extension and implements the `fminm`/`fmaxm` instructions and lowerings.

The Zfa extension provides additional floating point related instructions not included in either F/D extensions. Here's a [link to version 1 of the extension](https://github.com/riscv/riscv-isa-manual/blob/0902ff22aeb2cabbd41f9b06a4af0523fd5ecfb1/src/zfa.adoc) if anyone feels up to some light reading.

The `f{min,max}m` instructions implemented in this PR are pretty much the same as the base `fmin`/`fmax` present in the ISA, but they have different NaN behavior, that conveniently matches the semantics of Cranelift's `fmin`/`fmax` instructions, allowing for a more efficient lowering.




Patch:
diff --git a/cranelift/codegen/meta/src/isa/riscv64.rs b/cranelift/codegen/meta/src/isa/riscv64.rs
index 9a24efd91..fe4b545c0 100644
--- a/cranelift/codegen/meta/src/isa/riscv64.rs
+++ b/cranelift/codegen/meta/src/isa/riscv64.rs
@@ -56,6 +56,14 @@ pub(crate) fn define() -> TargetIsa {
         "Double-precision floating point",
         true,
     );
+
+    let _has_zfa = setting.add_bool(
+        "has_zfa",
+        "has extension Zfa?",
+        "Zfa: Extension for Additional Floating-Point Instructions",
+        false,
+    );
+
     let _has_v = setting.add_bool(
         "has_v",
         "has extension V?",
diff --git a/cranelift/codegen/src/isa/riscv64/inst.isle b/cranelift/codegen/src/isa/riscv64/inst.isle
index b77e66922..8a4d37959 100644
--- a/cranelift/codegen/src/isa/riscv64/inst.isle
+++ b/cranelift/codegen/src/isa/riscv64/inst.isle
@@ -597,6 +597,12 @@
   (FeqD)
   (FltD)
   (FleD)
+
+  ;; Zfa Extension
+  (FminmS)
+  (FmaxmS)
+  (FminmD)
+  (FmaxmD)
 ))
 
 
@@ -1035,6 +1041,9 @@
 (decl pure has_v () bool)
 (extern constructor has_v has_v)
 
+(decl pure has_zfa () bool)
+(extern constructor has_zfa has_zfa)
+
 (decl pure has_zbkb () bool)
 (extern constructor has_zbkb has_zbkb)
 
@@ -1554,6 +1563,17 @@
 (rule (rv_fmax $F32 rs1 rs2) (fpu_rrr (FpuOPRRR.FmaxS) $F32 (FRM.RTZ) rs1 rs2))
 (rule (rv_fmax $F64 rs1 rs2) (fpu_rrr (FpuOPRRR.FmaxD) $F64 (FRM.RTZ) rs1 rs2))
 
+;; `Zfa` Extension Instructions
+
+;; Helper for emitting the `fminm` instruction.
+(decl rv_fminm (Type FReg FReg) FReg)
+(rule (rv_fminm $F32 rs1 rs2) (fpu_rrr (FpuOPRRR.FminmS) $F32 (FRM.RDN) rs1 rs2))
+(rule (rv_fminm $F64 rs1 rs2) (fpu_rrr (FpuOPRRR.FminmD) $F64 (FRM.RDN) rs1 rs2))
+
+;; Helper for emitting the `fmaxm` instruction.
+(decl rv_fmaxm (Type FReg FReg) FReg)
+(rule (rv_fmaxm $F32 rs1 rs2) (fpu_rrr (FpuOPRRR.FmaxmS) $F32 (FRM.RUP) rs1 rs2))
+(rule (rv_fmaxm $F64 rs1 rs2) (fpu_rrr (FpuOPRRR.FmaxmD) $F64 (FRM.RUP) rs1 rs2))
 
 ;; `Zba` Extension Instructions
 
diff --git a/cranelift/codegen/src/isa/riscv64/inst/args.rs b/cranelift/codegen/src/isa/riscv64/inst/args.rs
index 4a0487889..ffa10121c 100644
--- a/cranelift/codegen/src/isa/riscv64/inst/args.rs
+++ b/cranelift/codegen/src/isa/riscv64/inst/args.rs
@@ -575,6 +575,10 @@ impl FpuOPRRR {
             Self::FeqD => "feq.d",
             Self::FltD => "flt.d",
             Self::FleD => "fle.d",
+            Self::FminmS => "fminm.s",
+            Self::FmaxmS => "fmaxm.s",
+            Self::FminmD => "fminm.d",
+            Self::FmaxmD => "fmaxm.d",
         }
     }
 
@@ -591,7 +595,9 @@ impl FpuOPRRR {
             | Self::FmaxS
             | Self::FeqS
             | Self::FltS
-            | Self::FleS => 0b1010011,
+            | Self::FleS
+            | Self::FminmS
+            | Self::FmaxmS => 0b1010011,
 
             Self::FaddD
             | Self::FsubD
@@ -604,7 +610,9 @@ impl FpuOPRRR {
             | Self::FmaxD
             | Self::FeqD
             | Self::FltD
-            | Self::FleD => 0b1010011,
+            | Self::FleD
+            | Self::FminmD
+            | Self::FmaxmD => 0b1010011,
         }
     }
 
@@ -636,6 +644,11 @@ impl FpuOPRRR {
             Self::FeqD => 0b1010001,
             Self::FltD => 0b1010001,
             Self::FleD => 0b1010001,
+
+            Self::FminmS => 0b0010100,
+            Self::FmaxmS => 0b0010100,
+            Self::FminmD => 0b0010101,
+            Self::FmaxmD => 0b0010101,
         }
     }
     pub fn is_32(self) -> bool {
diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle
index a0abd498d..bb8b14fc3 100644
--- a/cranelift/codegen/src/isa/riscv64/lower.isle
+++ b/cranelift/codegen/src/isa/riscv64/lower.isle
@@ -1744,13 +1744,19 @@
         (min FReg (rv_fmin ty x y)))
     (gen_select_freg is_ordered min nan)))
 
+;; With Zfa we can use the special `fminm` that precisely matches the expected
+;; NaN behavior.
+(rule 1 (lower (has_type (ty_scalar_float ty) (fmin x y)))
+  (if-let $true (has_zfa))
+  (rv_fminm ty x y))
+
 ;; vfmin does almost the right thing, but it does not handle NaN's correctly.
 ;; We should return a NaN if any of the inputs is a NaN, but vfmin returns the
 ;; number input instead.
 ;;
 ;; TODO: We can improve this by using a masked `fmin` instruction that modifies
 ;; the canonical nan register. That way we could avoid the `vmerge.vv` instruction.
-(rule 1 (lower (has_type (ty_vec_fits_in_register ty) (fmin x y)))
+(rule 2 (lower (has_type (ty_vec_fits_in_register ty) (fmin x y)))
   (let ((is_not_nan VReg (gen_fcmp_mask ty (FloatCC.Ordered) x y))
         (nan XReg (imm $I64 (canonical_nan_u64 (lane_type ty))))
         (vec_nan VReg (rv_vmv_vx nan ty))
@@ -1770,6 +1776,11 @@
         (max FReg (rv_fmax ty x y)))
     (gen_select_freg is_ordered max nan)))
 
+;; With Zfa we can use the special `fmaxm` that precisely matches the expected
+;; NaN behavior.
+(rule 1 (lower (has_type (ty_scalar_float ty) (fmax x y)))
+  (if-let $true (has_zfa))
+  (rv_fmaxm ty x y))
 
 ;; vfmax does almost the right thing, but it does not handle NaN's correctly.
 ;; We should return a NaN if any of the inputs is a NaN, but vfmax returns the
@@ -1777,7 +1788,7 @@
 ;;
 ;; TODO: We can improve this by using a masked `fmax` instruction that modifies
 ;; the canonical nan register. That way we could avoid the `vmerge.vv` instruction.
-(rule 1 (lower (has_type (ty_vec_fits_in_register ty) (fmax x y)))
+(rule 2 (lower (has_type (ty_vec_fits_in_register ty) (fmax x y)))
   (let ((is_not_nan VReg (gen_fcmp_mask ty (FloatCC.Ordered) x y))
         (nan XReg (imm $I64 (canonical_nan_u64 (lane_type ty))))
         (vec_nan VReg (rv_vmv_vx nan ty))
diff --git a/cranelift/codegen/src/isa/riscv64/lower/isle.rs b/cranelift/codegen/src/isa/riscv64/lower/isle.rs
index 3bdd6f0a8..64f32bd47 100644
--- a/cranelift/codegen/src/isa/riscv64/lower/isle.rs
+++ b/cranelift/codegen/src/isa/riscv64/lower/isle.rs
@@ -361,6 +361,10 @@ impl generated_code::Context for RV64IsleContext<'_, '_, MInst, Riscv64Backend>
         self.backend.isa_flags.has_m()
     }
 
+    fn has_zfa(&mut self) -> bool {
+        self.backend.isa_flags.has_zfa()
+    }
+
     fn has_zbkb(&mut self) -> bool {
         self.backend.isa_flags.has_zbkb()
     }


Test patch:
diff --git a/ci/build-test-matrix.js b/ci/build-test-matrix.js
index 92bfedfc2..38ad67c73 100644
--- a/ci/build-test-matrix.js
+++ b/ci/build-test-matrix.js
@@ -107,7 +107,7 @@ const array = [
     "target": "riscv64gc-unknown-linux-gnu",
     "gcc_package": "gcc-riscv64-linux-gnu",
     "gcc": "riscv64-linux-gnu-gcc",
-    "qemu": "qemu-riscv64 -cpu rv64,v=true,vlen=256,vext_spec=v1.0,zba=true,zbb=true,zbc=true,zbs=true,zbkb=true,zcb=true -L /usr/riscv64-linux-gnu",
+    "qemu": "qemu-riscv64 -cpu rv64,v=true,vlen=256,vext_spec=v1.0,Zfa=true,zba=true,zbb=true,zbc=true,zbs=true,zbkb=true,zcb=true -L /usr/riscv64-linux-gnu",
     "qemu_target": "riscv64-linux-user",
     "name": "Test Linux riscv64",
     "filter": "linux-riscv64",
diff --git a/cranelift/filetests/filetests/isa/riscv64/zfa.clif b/cranelift/filetests/filetests/isa/riscv64/zfa.clif
new file mode 100644
index 000000000..60193e684
--- /dev/null
+++ b/cranelift/filetests/filetests/isa/riscv64/zfa.clif
@@ -0,0 +1,70 @@
+test compile precise-output
+set unwind_info=false
+target riscv64 has_zfa
+
+
+function %fminm_s(f32, f32) -> f32 {
+block0(v0: f32, v1: f32):
+  v2 = fmin.f32 v0, v1
+  return v2
+}
+
+; VCode:
+; block0:
+;   fminm.s fa0,fa0,fa1,rdn
+;   ret
+;
+; Disassembled:
+; block0: ; offset 0x0
+;   .byte 0x53, 0x25, 0xb5, 0x28
+;   ret
+
+
+function %fminm_d(f64, f64) -> f64 {
+block0(v0: f64, v1: f64):
+  v2 = fmin.f64 v0, v1
+  return v2
+}
+
+; VCode:
+; block0:
+;   fminm.d fa0,fa0,fa1,rdn
+;   ret
+;
+; Disassembled:
+; block0: ; offset 0x0
+;   .byte 0x53, 0x25, 0xb5, 0x2a
+;   ret
+
+function %fmaxm_s(f32, f32) -> f32 {
+block0(v0: f32, v1: f32):
+  v2 = fmax.f32 v0, v1
+  return v2
+}
+
+; VCode:
+; block0:
+;   fmaxm.s fa0,fa0,fa1,rup
+;   ret
+;
+; Disassembled:
+; block0: ; offset 0x0
+;   .byte 0x53, 0x35, 0xb5, 0x28
+;   ret
+
+function %fmaxm_d(f64, f64) -> f64 {
+block0(v0: f64, v1: f64):
+  v2 = fmax.f64 v0, v1
+  return v2
+}
+
+; VCode:
+; block0:
+;   fmaxm.d fa0,fa0,fa1,rup
+;   ret
+;
+; Disassembled:
+; block0: ; offset 0x0
+;   .byte 0x53, 0x35, 0xb5, 0x2a
+;   ret
+
diff --git a/cranelift/filetests/filetests/runtests/fmax-pseudo.clif b/cranelift/filetests/filetests/runtests/fmax-pseudo.clif
index 2abf227f8..dcc923adb 100644
--- a/cranelift/filetests/filetests/runtests/fmax-pseudo.clif
+++ b/cranelift/filetests/filetests/runtests/fmax-pseudo.clif
@@ -4,6 +4,7 @@ target x86_64
 target x86_64 has_avx
 target aarch64
 target riscv64
+target riscv64 has_zfa
 target riscv64 has_c has_zcb
 target s390x
 
diff --git a/cranelift/filetests/filetests/runtests/fmax.clif b/cranelift/filetests/filetests/runtests/fmax.clif
index da7e23e04..e6d0edc83 100644
--- a/cranelift/filetests/filetests/runtests/fmax.clif
+++ b/cranelift/filetests/filetests/runtests/fmax.clif
@@ -5,6 +5,7 @@ target x86_64 has_avx
 target aarch64
 target s390x
 target riscv64
+target riscv64 has_zfa
 target riscv64 has_c has_zcb
 
 function %fmax_f32(f32, f32) -> f32 {
diff --git a/cranelift/filetests/filetests/runtests/fmin-pseudo.clif b/cranelift/filetests/filetests/runtests/fmin-pseudo.clif
index 44ef7b197..e6b60b427 100644
--- a/cranelift/filetests/filetests/runtests/fmin-pseudo.clif
+++ b/cranelift/filetests/filetests/runtests/fmin-pseudo.clif
@@ -4,6 +4,7 @@ target x86_64
 target x86_64 has_avx
 target aarch64
 target riscv64
+target riscv64 has_zfa
 target riscv64 has_c has_zcb
 target s390x
 
diff --git a/cranelift/filetests/filetests/runtests/fmin.clif b/cranelift/filetests/filetests/runtests/fmin.clif
index 72c171896..4e261502e 100644
--- a/cranelift/filetests/filetests/runtests/fmin.clif
+++ b/cranelift/filetests/filetests/runtests/fmin.clif
@@ -5,6 +5,7 @@ target x86_64 has_avx
 target aarch64
 target s390x
 target riscv64
+target riscv64 has_zfa
 target riscv64 has_c has_zcb
 
 function %fmin_f32(f32, f32) -> f32 {