[PATCH] D98848: [RISCV][Clang] Add RVV Vector Indexed Load intrinsic functions.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:90
+//      equivalent integer vector type with EEW and corresponding ELMUL (elmul =
+//      (eew/sew) * lmul). Fore example, vector type is __rvv_float16m4
+//      (SEW=16, LMUL=4) and Log2EEW is 3 (EEW=8), and then equivalent vector
----------------
Fore -> For


================
Comment at: clang/include/clang/Basic/riscv_vector.td:93
+//      type is __rvv_uint8m2_t (elmul=(8/16)*4 = 2). Ignore to define a new
+//      builtins if its qeuivalent type has illegal lmul.
 //
----------------
equivalent*


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:95
+  const std::string &getShortStr() {
+    if (ShortStr.empty())
+      initShortStr();
----------------
Why would it be empty? Don't we call initShortStr in the constructor? This at least needs a comment explaining why it is different than the other get*Str functions.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:621
+  // Extract and compute complex type transformer. It can only appear one time.
+  const Regex ComplexEx("\\((.*:.*)\\).*");
+  SmallVector<StringRef> Matches;
----------------
Do we really need a regex here? Can't we just check that it starts with '(' and then do a find for the ')'?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:626
+           "only allow one complex type transformer");
+    Transformer.consume_front(Twine("(" + Matches[1] + ")").str());
+    auto UpdateAndCheckComplexProto = [&]() {
----------------
Can we do

```
Transformer = Transformer.drop_front(Matches[1].size() + 2);
```


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1011
+    size_t Idx = 0;
+    // Bypass complex prototype because it could have primitive type
+    // char.
----------------
I'd write this as "Skip over complex prototype because it could contain primitive type character."


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1026
+  parsePrototypes(Prototypes, SuffixStrs,
+                  [&](StringRef Proto, SmallVector<std::string> &Result) {
+                    auto T = computeType(Type, Log2LMUL, Proto);
----------------
You don't need to pass SuffixStrs into parsePrototypes you can just capture it in the lambda and use SuffixStrs.push_back() in the lambda.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1066
+    parsePrototypes(Prototypes, ProtoSeq,
+                    [](StringRef Proto, SmallVector<std::string> &Result) {
+                      Result.push_back(Proto.str());
----------------
Just capture ProtoSeq in the lambda capture list and use ProtoSeq.push_back in the body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98848