[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

Zakk Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 13 09:19:25 PDT 2022


khchen added a comment.

Do we need to have some tests in `clang/test/PCH/` for new #pragma?



================
Comment at: clang/lib/Sema/SemaLookup.cpp:932
+      if (DeclareRVVBuiltins) {
+        if (GetRVVBuiltinInfo(*this, R, II, PP)) {
+          return true;
----------------
Don’t Use Braces on Simple Single-Statement Bodies.


================
Comment at: clang/lib/Support/RISCVVIntrinsicUtils.cpp:884
 RVVIntrinsic::getSuffixStr(BasicType Type, int Log2LMUL,
-                           const llvm::SmallVector<TypeProfile> &TypeProfiles) {
+                           const llvm::ArrayRef<TypeProfile> &TypeProfiles) {
   SmallVector<std::string> SuffixStrs;
----------------
maybe this changed should be in another NFC patch.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:372
     StringRef Name = R->getValueAsString("Name");
-    StringRef SuffixProto = R->getValueAsString("Suffix");
+    StringRef Suffix = R->getValueAsString("Suffix");
     StringRef MangledName = R->getValueAsString("MangledName");
----------------
maybe all renaming stuffs should be in another NFC patch.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:480
+    // They are handled by riscv_vector.h
+    if (Name == "vsetvli" || Name == "vsetvlimax")
+      continue;
----------------
I feel little tricky to checking the name here. what do you mean they are handled by riscv_vector.h?
do you mean they have `vsetvl_macro:RVVHeader`?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:611
+  for (const auto &SR : SemaRecords) {
+    // Output *MUST* sync with RVVIntrinsicRecord in SemaRVVLookup.cpp.
+    OS << "{"
----------------
I'm thinking is it possible to have an unittest or test to make sure we won't screw up in the future implementation?
Is it possible to have unittest to test implement really have `sync` correctly?
Is it easy to debug the mismatch problem during implementation without any new test added?
We will add a new implementation (really cool speed up and meaningful improvement), but unfortunately we don't have any tests, that make me a little hesitating...

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111617/new/

https://reviews.llvm.org/D111617



More information about the llvm-commits mailing list