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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Some drive-by comments from the peanut gallery.



================
Comment at: clang/lib/Parse/ParsePragma.cpp:3963
+        << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'intrinsic'";
+    return;
+  }
----------------
It's fine to warn on this, but then you need to eat tokens until the end of directive is found so that parsing recovery is correct. e.g.,
```
#pragma clang riscv int i = 12;
```
See `HandlePragmaAttribute()` for an example (though you'll look for `eod` instead of `eof`).


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3971
+        << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'vector'";
+    return;
+  }
----------------
Same here.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3978
+        << "clang riscv intrinsic";
+    return;
+  }
----------------
And here as well.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:100
+    switch (Type->getElementBitwidth()) {
+    case 64:
+      QT = Context.DoubleTy;
----------------
I almost hate to ask, but... `long double`? Any of the 16-bit float types?


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:111
+    break;
+  default:
+    llvm_unreachable("Unhandled type.");
----------------
Might as well handle `Invalid` and then drop the `default` entirely so it's a fully covered switch.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:121
+  // Transform the type to a pointer as the last step, if necessary.
+  if (Type->isPointer())
+    QT = Context.getPointerType(QT);
----------------
Double-checking -- do you have to care about references as well?


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:185-186
+                                                   Record.OverloadedSuffixSize);
+    for (int TypeRangeMaskShift = 0;
+         TypeRangeMaskShift <= static_cast<int>(BasicType::MaxOffset);
+         ++TypeRangeMaskShift) {
----------------
Given that we're bit twiddling with this, I'd feel more comfortable if this was `unsigned int` rather than `int` (same for `BaseTypeI`).


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:223-225
+        if (!Types.hasValue()) {
+          continue;
+        }
----------------



================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:227-229
+        auto SuffixStr =
+            RVVIntrinsic::getSuffixStr(BaseType, Log2LMUL, SuffixProto);
+        auto OverloadedSuffixStr = RVVIntrinsic::getSuffixStr(
----------------
You should spell the type explicitly here instead of using `auto`.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:235-237
+        bool HasMask = Record.MaskedPrototypeLength != 0;
+
+        if (HasMask) {
----------------



================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:299-301
+  auto Sigs = IDef.Signature;
+  size_t SigLength = Sigs.size();
+  auto ReturnType = Sigs[0];
----------------
Spell out the types instead of using `auto`.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:322
+      SC_Extern, S.getCurFPFeatures().isFPConstrained(), false,
+      BuiltinFuncType->isFunctionProtoType());
+
----------------
No need to calculate this, we already know.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:328
+  SmallVector<ParmVarDecl *, 8> ParmList;
+  for (unsigned IParm = 0, e = FP->getNumParams(); IParm != e; ++IParm) {
+    ParmVarDecl *Parm =
----------------
Naming style fix.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:342
+  // Setup alias to __builtin_rvv_*
+  auto &IntrinsicII = PP.getIdentifierTable().get(IDef.BuiltinName);
+  RVVIntrinsicDecl->addAttr(
----------------
Spell out type name.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:358
+  if (OvIItr != OverloadIntrinsics.end()) {
+    auto OvIntrinsicDef = OvIItr->second;
+    for (auto Index : OvIntrinsicDef.Indexs)
----------------
Spell out the type.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:41
+
+  // Supported type, mask of BasicType
+  unsigned TypeRangeMask;
----------------



================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:74
+public:
+  static constexpr unsigned INVALID_INDEX = (unsigned)-1;
+
----------------



================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:232-237
+  for (const auto &SemaRecord : SemaRecords) {
+    InsertToSignatureSet(SemaRecord.Prototype);
+    InsertToSignatureSet(SemaRecord.MaskedPrototype);
+    InsertToSignatureSet(SemaRecord.Suffix);
+    InsertToSignatureSet(SemaRecord.OverloadedSuffix);
   }
----------------



================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:239-240
+
+  for (const auto &Sig : Signatures)
+    insert(Sig);
+}
----------------
Pretty sure you can go with this instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617