khchen added a comment.
Herald added a subscriber: eopXD.
It seems like not only one place need to have a consistent way to process
intrinsic. (ex. InitIntrinsicList/createRVVIntrinsics and
RVVIntrinsic::RVVIntrinsic/InitRVVIntrinsic)
I'm think how to avoid mismatch implementation in the future, because we will
support a lot of new builtin with tail and mask policy...
================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:91
+struct RVVIntrinsicDef {
+ std::string Name;
+ std::string GenericName;
----------------
why do we need to declare Name as std::string here but RVVIntrinsicRecord use
`const char*`?
================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:92
+ std::string Name;
+ std::string GenericName;
+ std::string BuiltinName;
----------------
Nit: I think we use the `overload` terminology rather than `generic`.
================
Comment at: clang/lib/Sema/SemaRVVLookup.cpp:359-371
+ if (!Record.MangledName)
+ MangledName = StringRef(Record.Name).split("_").first.str();
+ else
+ MangledName = Record.MangledName;
+ if (!SuffixStr.empty())
+ Name += "_" + SuffixStr.str();
+ if (!MangledSuffixStr.empty())
----------------
IIUC, above code initialize the BuiltinName, Name and MangledName same with
RVVIntrinsic::RVVIntrinsic did, right?
If yes, I think we need to have some comment note that.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:143
+ /// Emit all the information needed by SemaLookup.cpp.
+ void createSema(raw_ostream &o);
+
----------------
It will be good to have a description about why we need to have custom
SemaLookup.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:152
void createRVVHeaders(raw_ostream &OS);
+ ///
+ unsigned GetSemaSignatureIndex(const SmallVector<std::string> &Signature);
----------------
missed comment?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:163
- // Emit the architecture preprocessor definitions. Return true when emits
- // non-empty string.
- bool emitExtDefStr(uint8_t Extensions, raw_ostream &o);
+#if 0
// Slice Prototypes string into sub prototype string and process each sub
----------------
Forget to remove this code?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:171
+ void EmitSemaRecords(raw_ostream &OS);
+ void ConstructSemaSignatureTable();
+ void EmitSemaSignatureTable(raw_ostream &OS);
----------------
Could we have comment for signature table.
IIUC, it stores ProtoSeq, ProtoMaskSeq, SuffixProto and MangledSuffixProto
information in each entry, and SemaRecord use the index to get the information
from signature table, right?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:853
+
+ for (auto SemaRecord : SemaRecords) {
+ InsertToSignatureSet(SemaRecord.ProtoSeq);
----------------
Use constant reference to access range-based loop?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:860
+
+ for (auto Sig : Signatures) {
+ GetSemaSignatureIndex(Sig);
----------------
Don’t Use Braces on Simple Single-Statement Bodies of if/else/loop Statements.
Please check your code again.
================
Comment at: llvm/include/llvm/Support/RISCVVIntrinsicUtils.h:23
+
+enum RVVBasicType {
+ RVVBasicTypeUnknown = 0,
----------------
I think using typed enums is clearer because we would use TypeRangeMask to
record supported basic types.
It should have the same type with TypeRangeMask.
================
Comment at: llvm/include/llvm/Support/RISCVVIntrinsicUtils.h:84
+ RVVTypeModifierLMUL1 = 1 << 6,
+ RVVTypeModifierMaskMax = RVVTypeModifierLMUL1,
+};
----------------
different naming rule and initialize way comparing to `RVVBasicTypeMaxOffset`
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
