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