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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/Basic/TokenKinds.def:911
 
+// Annotation for the riscv pragma directives - #pragma clang riscv intrinsic ..
+PRAGMA_ANNOTATION(pragma_riscv)
----------------
Why only 2 periods at the end. Should be 3 like on like 908 or in the middle of line 903


================
Comment at: clang/include/clang/Sema/Sema.h:1590
 
+  /// Indicate RVV builtin funtions enabled or not.
+  bool DeclareRVVBuiltins = false;
----------------
funtion -> functions


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:110
   }
+  bool operator==(const PrototypeDescriptor &PD) const {
+    return !(*this != PD);
----------------
I think it's more conventional to define operator== as `PD.PT == PT && PD.VTM == VTM && PD.TM == TM` and operator!= as `!(*this == PD)`


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:114
   bool operator>(const PrototypeDescriptor &PD) const {
-    return !(PD.PT <= PT && PD.VTM <= VTM && PD.TM <= TM);
+    if (PD.PT != PT)
+      return PD.PT > PT;
----------------
This can be written as

`return std::tie(PD.PT, PD.VTM, PD.TM) > std::tie(PT, VTM, TM);`

Though it's still surprising that PD is on the left. This is operator> but the implementation looks more like operator<.


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:364
+
+  // Overloaded intrinsic name, could be empty if can be computed from Name
+  // e.g. vadd
----------------
"if can" -> "if it can"


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:395
+
+  // Supported type, mask of BasicType
+  uint8_t TypeRangeMask;
----------------
Missing period at end of comment.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3960
+  IdentifierInfo *II = Tok.getIdentifierInfo();
+  if (!II || (!II->isStr("intrinsic"))) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
----------------
Drop parenthese arounds `(!II->isStr("intrinsic"))`


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3968
+  II = Tok.getIdentifierInfo();
+  if (!II || (!II->isStr("vector"))) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
----------------
Drop parentheses around `(!II->isStr("vector"))`


================
Comment at: clang/lib/Sema/CMakeLists.txt:49
   SemaLookup.cpp
+  SemaRISCVVectorLookup.cpp
   SemaModule.cpp
----------------
These are supposed to be in alphabetical order


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:68
+                                                       uint8_t Length) {
+  return ArrayRef<PrototypeDescriptor>(&RVVSignatureTable[Index], Length);
+}
----------------
Can this use makeArrayRef?


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:117
+
+  if (Type->isConstant()) {
+    QT = Context.getConstType(QT);
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:122
+  // Transform the type to a pointer as the last step, if necessary.
+  if (Type->isPointer()) {
+    QT = Context.getPointerType(QT);
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:262
+
+  if (IsMask) {
+    Name += "_m";
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:277
+  std::string BuiltinName = "__builtin_rvv_" + std::string(Record.Name);
+  if (IsMask) {
+    BuiltinName += "_m";
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:311
+  // Skip return type, and convert RVVType to QualType for arguments.
+  for (size_t i = 1; i < SigLength; ++i) {
+    ArgTypes.push_back(RVVType2Qual(Context, Sigs[i]));
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:364
+    auto OvIntrinsicDef = OvIItr->second;
+    for (auto Index : OvIntrinsicDef.Indexs) {
+      CreateRVVIntrinsicDecl(S, LR, II, PP, Index,
----------------
Drop curly braces


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:382
+
+  // It's not RVV intrinsics.
+  return false;
----------------
"It's not an RVV intrinsic."


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:389
+                       Preprocessor &PP) {
+  static std::unique_ptr<RVVIntrinsicManager> IntrinsicManager =
+      std::make_unique<RVVIntrinsicManager>(S.Context);
----------------
Should this be a member of Sema instead of a function static? RVVIntrinsicManager holds a reference to ASTContext but will outlive it.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:76
+
+  // Create compressed hsignature table from SemaRecords.
+  void init(const std::vector<SemaRecord> &SemaRecords);
----------------
Is `hsignature` a typo?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:582
+      BasicType TypeRangeMask = BasicType::Unknown;
+      for (char I : TypeRange) {
+        TypeRangeMask |= ParseBasicType(I);
----------------
Drop curly braces


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:588
+      unsigned Log2LMULMask = 0;
+      for (int Log2LMUL : Log2LMULList) {
+        Log2LMULMask |= 1 << (Log2LMUL + 3);
----------------
Drop curly braces


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:611
+
+      auto InitSuffixtype = [&](SmallVectorImpl<PrototypeDescriptor> &PS,
+                                StringRef Prototypes) {
----------------
This lambda doesn't seem to provide much value.

What's wrong with

```
SR.Suffix = parsePrototypes(SuffixProto);
SR.OverloadedSuffix = parsePrototypes(OverloadedSuffixProto);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617