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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:511
+    RISCVPragmaHandler = std::make_unique<PragmaRISCVHandler>(Actions);
+    PP.AddPragmaHandler(RISCVPragmaHandler.get());
+  }
----------------
Since this is a clang specific pragma should it be `#pragma clang riscv intrinsic`? gcc's pragma for SVE is `#pragma GCC aarch64 "arm_sve.h"`


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3842
+  }
+
+  PP.setPredefines("#define __riscv_pragma_vector_intrinsics");
----------------
Do we need to check that there are no tokens left to parse? Quick look at other handles I see a check for tok::eod


================
Comment at: clang/lib/Sema/SemaLookup.cpp:904
+                                      QualType &RetType,
+                                      SmallVector<QualType, 5> &ArgTypes) {
+  // Get the QualType instance of the return type.
----------------
Can this be SmallVectorImpl?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:921
+                                  QualType &BuiltinFuncType, QualType &RetType,
+                                  SmallVector<QualType, 5> &ArgTypes) {
+  FunctionProtoType::ExtProtoInfo PI(
----------------
SmallVectorImpl


================
Comment at: clang/lib/Sema/SemaLookup.cpp:929
+
+static unsigned GetTargetFeatures(const TargetInfo &TI) {
+  bool HasF = TI.hasFeature("f");
----------------
Use a fixed size type like uint32_t since it is a bit vector.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:949
+    Sema &S, LookupResult &LR, IdentifierInfo *II, Preprocessor &PP,
+    const unsigned FctIndex, const unsigned Len, const unsigned BuiltinIndex) {
+
----------------
const on integer arguments doesn't make a much sense. It just prevents you from assigning over the variable in the function body.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:966
+  ASTContext &Context = S.Context;
+  unsigned RVVTargetFeatures = GetTargetFeatures(Context.getTargetInfo());
+
----------------
Use a fixed size type like uint32_t here or uint8_t to match RequiredExts,


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1513
+  OS << "enum RVVTypeID {\n";
+  StringMap<bool> Seen;
+  for (const auto &T : TypeMap) {
----------------
Use StringSet?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1516
+    auto *RVVTy = T.first;
+    if (Seen.find(RVVTy->getShortStr()) == Seen.end()) {
+      OS << "  RVVT_" + RVVTy->getShortStr() << ",\n";
----------------
With StringSet you can use
```
if (Seen.insert(RVVTy->getShortStr()).second)
```

The .second field from the pair will tell if you the insert happened or not.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1551
+    StringRef IName = Def->getName();
+    if (FctOverloadMap.find(IName) == FctOverloadMap.end()) {
+      FctOverloadMap.insert(std::make_pair(
----------------
I don't think you need to insert anything here. The `FctOverloadMap[IName].push_back(` line below will construct an empty entry before the push_back if it doesn't already exist.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1575
+      StringRef GName = Def->getMangledName();
+      if (FctOverloadMap.find(GName) == FctOverloadMap.end()) {
+        FctOverloadMap.insert(std::make_pair(
----------------
I don't think you need to insert anything. The push_back line will take care of it.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1616
+  // List of signatures known to be emitted.
+  std::vector<BuiltinIndexListTy *> KnownSignatures;
+
----------------
Can this be a vector of unique_ptrs?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1622
+    // Gather all signatures for the current function.
+    auto *CurSignatureList = new BuiltinIndexListTy();
+    for (const auto &Signature : Fct.second)
----------------
Can this be a unique_ptr?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1639
+          *Candidate == *CurSignatureList) {
+        if (CanReuseSignature(Candidate, Fct.second)) {
+          SignatureListMap.find(Candidate)->second.Names.push_back(Fct.first);
----------------
I think this condition can be an additional && on the previous if?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1640
+        if (CanReuseSignature(Candidate, Fct.second)) {
+          SignatureListMap.find(Candidate)->second.Names.push_back(Fct.first);
+          SignatureListMap.find(Candidate)->second.BuiltinIndex.push_back(
----------------
Don't call SignatureListMap.find(Candidate) twice.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1677
+        // Report an error when seeing an entry that is too large for the
+        // current index type (unsigned short).  When hitting this, the type
+        // of SignatureTable will need to be changed.
----------------
I'd use uint16_t rather than unsigned short.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1721
+void RVVEmitter::EmitBuiltinMapTable(raw_ostream &OS) {
+  OS << "static std::map<std::pair<StringRef, unsigned>, unsigned> ";
+  OS << " RVVGenericBuiltinMap = {\n";
----------------
I believe this will create a static global constructor which is not allowed by coding standards https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1760
+//          matching an RVV builtin function.
+static std::tuple<unsigned, unsigned, unsigned> isRVVBuiltin(llvm::StringRef Name) {
+
----------------
isRVVBuiltin isn't a good name if it doesn't return a bool.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1784
+  // are added in step 2.
+  StringMap<bool> TypesSeen;
+  for (const auto &T : TypeMap) {
----------------
StringSet?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617