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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
