Skip to content

[RISCV][sema] Correct the requirement of vf[n|w]cvt.x[|u].f intrinsics #101811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

4vtomat
Copy link
Member

@4vtomat 4vtomat commented Aug 3, 2024

Fix #101526

vf[n|w]cvt.x[|u].f for f16 needs zvfh instead of zvfhmin, current approach
is not able to detect this. Ultimately we need to add zvfh to RequiredFeatures
to check other intrinsics instead, the type check should be done in checkRVVTypeSupport.

Fix llvm#101526

`vf[n|w]cvt.x[|u].f` for f16 needs `zvfh` instead of `zvfhmin`, current approach
is not able to detect this. Ultimately we need to add `zvfh` to RequiredFeatures
to check other intrinsics instead, the type check should be done in checkRVVTypeSupport.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 3, 2024

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-clang

Author: Brandon Wu (4vtomat)

Changes

Fix #101526

vf[n|w]cvt.x[|u].f for f16 needs zvfh instead of zvfhmin, current approach
is not able to detect this. Ultimately we need to add zvfh to RequiredFeatures
to check other intrinsics instead, the type check should be done in checkRVVTypeSupport.


Full diff: https://github.com/llvm/llvm-project/pull/101811.diff

5 Files Affected:

  • (modified) clang/include/clang/Basic/riscv_vector.td (+42-6)
  • (modified) clang/include/clang/Basic/riscv_vector_common.td (+2-2)
  • (modified) clang/include/clang/Support/RISCVVIntrinsicUtils.h (+2-1)
  • (modified) clang/lib/Sema/SemaRISCV.cpp (+6)
  • (modified) clang/utils/TableGen/RISCVVEmitter.cpp (+1)
diff --git a/clang/include/clang/Basic/riscv_vector.td b/clang/include/clang/Basic/riscv_vector.td
index 0cab4b8067f0d..662771d640b69 100644
--- a/clang/include/clang/Basic/riscv_vector.td
+++ b/clang/include/clang/Basic/riscv_vector.td
@@ -1912,8 +1912,18 @@ def vfcvt_rtz_x_f_v : RVVConvToSignedBuiltin<"vfcvt_rtz_x">;
 let Log2LMUL = [-3, -2, -1, 0, 1, 2] in {
   def vfwcvt_rtz_xu_f_v : RVVConvToWidenUnsignedBuiltin<"vfwcvt_rtz_xu">;
   def vfwcvt_rtz_x_f_v : RVVConvToWidenSignedBuiltin<"vfwcvt_rtz_x">;
-  def vfwcvt_f_xu_v : RVVConvBuiltin<"Fw", "FwUv", "csi", "vfwcvt_f">;
-  def vfwcvt_f_x_v : RVVConvBuiltin<"Fw", "Fwv", "csi", "vfwcvt_f">;
+  def vfwcvt_f_xu_v : RVVConvBuiltin<"Fw", "FwUv", "si", "vfwcvt_f">;
+  def vfwcvt_f_x_v : RVVConvBuiltin<"Fw", "Fwv", "si", "vfwcvt_f">;
+  let RequiredFeatures = ["Zvfh"] in {
+    let Name = "vfwcvt_f_xu_v",
+        IRName = "vfwcvt_f_xu_v",
+        MaskedIRName = "vfwcvt_f_xu_v_mask" in
+      def : RVVConvBuiltin<"Fw", "FwUv", "c", "vfwcvt_f">;
+    let Name = "vfwcvt_f_x_v",
+        IRName = "vfwcvt_f_x_v",
+        MaskedIRName = "vfwcvt_f_x_v_mask" in
+      def : RVVConvBuiltin<"Fw", "Fwv", "c", "vfwcvt_f">;
+  }
   def vfwcvt_f_f_v : RVVConvBuiltin<"w", "wv", "f", "vfwcvt_f">;
   let RequiredFeatures = ["Zvfhmin"] in
     def vfwcvt_f_f_v_fp16 : RVVConvBuiltin<"w", "wv", "x", "vfwcvt_f"> {
@@ -1927,6 +1937,16 @@ let Log2LMUL = [-3, -2, -1, 0, 1, 2] in {
 let Log2LMUL = [-3, -2, -1, 0, 1, 2] in {
   def vfncvt_rtz_xu_f_w : RVVConvToNarrowingUnsignedBuiltin<"vfncvt_rtz_xu">;
   def vfncvt_rtz_x_f_w : RVVConvToNarrowingSignedBuiltin<"vfncvt_rtz_x">;
+  let RequiredFeatures = ["Zvfh"] in {
+    let Name = "vfncvt_rtz_xu_f_w",
+        IRName = "vfncvt_rtz_xu_f_w",
+        MaskedIRName = "vfncvt_rtz_xu_f_w_mask" in
+      def : RVVConvBuiltin<"Uv", "UvFw", "c", "vfncvt_rtz_xu">;
+    let Name = "vfncvt_rtz_x_f_w",
+        IRName = "vfncvt_rtz_x_f_w",
+        MaskedIRName = "vfncvt_rtz_x_f_w_mask" in
+      def : RVVConvBuiltin<"Iv", "IvFw", "c", "vfncvt_rtz_x">;
+  }
   def vfncvt_rod_f_f_w : RVVConvBuiltin<"v", "vw", "xf", "vfncvt_rod_f">;
 }
 
@@ -2005,10 +2025,18 @@ let ManualCodegen = [{
     let Log2LMUL = [-3, -2, -1, 0, 1, 2] in {
       let OverloadedName = "vfncvt_x" in
         defm :
-          RVVConvBuiltinSet<"vfncvt_x_f_w", "csi", [["Iv", "IvFwu"]]>;
+          RVVConvBuiltinSet<"vfncvt_x_f_w", "si", [["Iv", "IvFwu"]]>;
       let OverloadedName = "vfncvt_xu" in
         defm :
-          RVVConvBuiltinSet<"vfncvt_xu_f_w", "csi", [["Uv", "UvFwu"]]>;
+          RVVConvBuiltinSet<"vfncvt_xu_f_w", "si", [["Uv", "UvFwu"]]>;
+      let RequiredFeatures = ["Zvfh"] in {
+        let OverloadedName = "vfncvt_x" in
+          defm :
+            RVVConvBuiltinSet<"vfncvt_x_f_w", "c", [["Iv", "IvFwu"]]>;
+        let OverloadedName = "vfncvt_xu" in
+          defm :
+            RVVConvBuiltinSet<"vfncvt_xu_f_w", "c", [["Uv", "UvFwu"]]>;
+      }
       let OverloadedName = "vfncvt_f" in {
         defm :
           RVVConvBuiltinSet<"vfncvt_f_x_w", "xf", [["v", "vIwu"]]>;
@@ -2055,10 +2083,18 @@ let ManualCodegen = [{
   let Log2LMUL = [-3, -2, -1, 0, 1, 2] in {
     let OverloadedName = "vfncvt_x" in
       defm :
-        RVVConvBuiltinSet<"vfncvt_x_f_w", "csi", [["Iv", "IvFw"]]>;
+        RVVConvBuiltinSet<"vfncvt_x_f_w", "si", [["Iv", "IvFw"]]>;
     let OverloadedName = "vfncvt_xu" in
       defm :
-        RVVConvBuiltinSet<"vfncvt_xu_f_w", "csi", [["Uv", "UvFw"]]>;
+        RVVConvBuiltinSet<"vfncvt_xu_f_w", "si", [["Uv", "UvFw"]]>;
+    let RequiredFeatures = ["Zvfh"] in {
+      let OverloadedName = "vfncvt_x" in
+        defm :
+          RVVConvBuiltinSet<"vfncvt_x_f_w", "c", [["Iv", "IvFw"]]>;
+      let OverloadedName = "vfncvt_xu" in
+        defm :
+          RVVConvBuiltinSet<"vfncvt_xu_f_w", "c", [["Uv", "UvFw"]]>;
+    }
     let OverloadedName = "vfncvt_f" in {
       defm :
         RVVConvBuiltinSet<"vfncvt_f_x_w", "xf", [["v", "vIw"]]>;
diff --git a/clang/include/clang/Basic/riscv_vector_common.td b/clang/include/clang/Basic/riscv_vector_common.td
index 040db6f0cdbfb..33f6441217a5e 100644
--- a/clang/include/clang/Basic/riscv_vector_common.td
+++ b/clang/include/clang/Basic/riscv_vector_common.td
@@ -604,10 +604,10 @@ class RVVConvToWidenUnsignedBuiltin<string overloaded_name>
     : RVVConvBuiltin<"Uw", "Uwv", "xf", overloaded_name>;
 
 class RVVConvToNarrowingSignedBuiltin<string overloaded_name>
-    : RVVConvBuiltin<"Iv", "IvFw", "csi", overloaded_name>;
+    : RVVConvBuiltin<"Iv", "IvFw", "si", overloaded_name>;
 
 class RVVConvToNarrowingUnsignedBuiltin<string overloaded_name>
-    : RVVConvBuiltin<"Uv", "UvFw", "csi", overloaded_name>;
+    : RVVConvBuiltin<"Uv", "UvFw", "si", overloaded_name>;
 
 let HasMaskedOffOperand = true in {
   multiclass RVVSignedReductionBuiltin {
diff --git a/clang/include/clang/Support/RISCVVIntrinsicUtils.h b/clang/include/clang/Support/RISCVVIntrinsicUtils.h
index b4ff61784126e..9a6a2092eb996 100644
--- a/clang/include/clang/Support/RISCVVIntrinsicUtils.h
+++ b/clang/include/clang/Support/RISCVVIntrinsicUtils.h
@@ -502,7 +502,8 @@ enum RVVRequire : uint32_t {
   RVV_REQ_Zvksh = 1 << 15,
   RVV_REQ_Zvfbfwma = 1 << 16,
   RVV_REQ_Zvfbfmin = 1 << 17,
-  RVV_REQ_Experimental = 1 << 18,
+  RVV_REQ_Zvfh = 1 << 18,
+  RVV_REQ_Experimental = 1 << 19,
 
   LLVM_MARK_AS_BITMASK_ENUM(RVV_REQ_Experimental)
 };
diff --git a/clang/lib/Sema/SemaRISCV.cpp b/clang/lib/Sema/SemaRISCV.cpp
index f1c7c0516e671..abf8e4ac2f3e8 100644
--- a/clang/lib/Sema/SemaRISCV.cpp
+++ b/clang/lib/Sema/SemaRISCV.cpp
@@ -222,6 +222,7 @@ void RISCVIntrinsicManagerImpl::ConstructRVVIntrinsics(
       {"zvksh", RVV_REQ_Zvksh},
       {"zvfbfwma", RVV_REQ_Zvfbfwma},
       {"zvfbfmin", RVV_REQ_Zvfbfmin},
+      {"zvfh", RVV_REQ_Zvfh},
       {"experimental", RVV_REQ_Experimental}};
 
   // Construction of RVVIntrinsicRecords need to sync with createRVVIntrinsics
@@ -280,6 +281,11 @@ void RISCVIntrinsicManagerImpl::ConstructRVVIntrinsics(
       if ((BaseTypeI & Record.TypeRangeMask) != BaseTypeI)
         continue;
 
+      // TODO: Remove the check below and use RequiredFeatures in
+      // riscv_vector.td to check the intrinsics instead, the type check should
+      // be done in checkRVVTypeSupport. This check also not able to work on the
+      // intrinsics that have Float16 but the BaseType is not Float16 such as
+      // `vfcvt_f_x_v`.
       if (BaseType == BasicType::Float16) {
         if ((Record.RequiredExtensions & RVV_REQ_Zvfhmin) == RVV_REQ_Zvfhmin) {
           if (!TI.hasFeature("zvfhmin"))
diff --git a/clang/utils/TableGen/RISCVVEmitter.cpp b/clang/utils/TableGen/RISCVVEmitter.cpp
index 7f3cb70c97d09..ef7159fae9fd2 100644
--- a/clang/utils/TableGen/RISCVVEmitter.cpp
+++ b/clang/utils/TableGen/RISCVVEmitter.cpp
@@ -670,6 +670,7 @@ void RVVEmitter::createRVVIntrinsics(
               .Case("Zvksh", RVV_REQ_Zvksh)
               .Case("Zvfbfwma", RVV_REQ_Zvfbfwma)
               .Case("Zvfbfmin", RVV_REQ_Zvfbfmin)
+              .Case("Zvfh", RVV_REQ_Zvfh)
               .Case("Experimental", RVV_REQ_Experimental)
               .Default(RVV_REQ_None);
       assert(RequireExt != RVV_REQ_None && "Unrecognized required feature?");

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@4vtomat 4vtomat merged commit 40c2aaf into llvm:main Aug 6, 2024
11 checks passed
@4vtomat 4vtomat deleted the add_req_ext_fpTointConversion branch August 6, 2024 10:00
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Aug 29, 2024
…25b660a80

Local branch amd-gfx 86625b6 Merged main:b7730a23efb222944b732bbdb3a7b965b7bffd98 into amd-gfx:0616aed9b07e
Remote branch main 40c2aaf [RISCV][sema] Correct the requirement of `vf[n|w]cvt.x[|u].f` intrinsics (llvm#101811)
tclin914 added a commit that referenced this pull request Nov 11, 2024
This is a follow-up patch for
#101811.
That we can remove the type checking for fp16 from SemaRISCV.cpp.

Fixes: #101621 and
#94306
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
This is a follow-up patch for
llvm#101811.
That we can remove the type checking for fp16 from SemaRISCV.cpp.

Fixes: llvm#101621 and
llvm#94306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RISCV] float to int conversion intrinsics are incorrectly enabled with Zvfhmin
3 participants