khchen added inline comments.
================ Comment at: clang/lib/Sema/SemaRVVLookup.cpp:175 + for (auto &Record : RVVIntrinsicRecords) { + // Create Intrinsics for each type and LMUL. + BasicType BaseType = BasicType::Unknown; ---------------- Those code logic need to sync with createRVVIntrinsics, maybe we could add more comment address that. ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:91 private: /// Create all intrinsics and add them to \p Out void createRVVIntrinsics(std::vector<std::unique_ptr<RVVIntrinsic>> &Out); ---------------- and also init SemaRecords? ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:93 void createRVVIntrinsics(std::vector<std::unique_ptr<RVVIntrinsic>> &Out); + /// Create all intrinsics record from RVVIntrinsics. + void createRVVIntrinsicRecord(std::vector<RVVIntrinsicRecord> &Out); ---------------- I think it should be "Create all intrinsics record and SemaSignatureTable from SemaRecords"? ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:99 - /// Emit Acrh predecessor definitions and body, assume the element of Defs are - /// sorted by extension. - void emitArchMacroAndBody( - std::vector<std::unique_ptr<RVVIntrinsic>> &Defs, raw_ostream &o, - std::function<void(raw_ostream &, const RVVIntrinsic &)>); - - // Emit the architecture preprocessor definitions. Return true when emits - // non-empty string. - bool emitMacroRestrictionStr(RISCVPredefinedMacroT PredefinedMacros, - raw_ostream &o); + /// Construct a compressed signature table used for createSema. + void ConstructSemaSignatureTable(); ---------------- /// Construct a compressed signature table from SemaRecords which is used for createSema. maybe better. ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:103 + unsigned + GetSemaSignatureIndex(const SmallVector<PrototypeDescriptor> &Signature); }; ---------------- This is ambiguous of naming and comment because it also insert signature into SemaSignatureTable if not found in the table. ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:597 + }; + + for (const auto &SemaRecord : SemaRecords) { ---------------- maybe we need an assert to check SemaRecords is not empty. ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:637 + std::vector<RVVIntrinsicRecord> RVVIntrinsicRecords; + createRVVIntrinsics(Defs); + ---------------- we only need SemaRecords initialization part of createRVVIntrinsics, right? ================ Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:639 + + createRVVIntrinsicRecord(RVVIntrinsicRecords); + ---------------- `createRVVIntrinsicRecord also init SemaSignatureTable implicitly.` 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