aaron.ballman added a comment.
Some drive-by comments from the peanut gallery.
================
Comment at: clang/lib/Parse/ParsePragma.cpp:3963
+ << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true <<
"'intrinsic'";
+ return;
+ }
----------------
It's fine to warn on this, but then you need to eat tokens until the end of
directive is found so that parsing recovery is correct. e.g.,
```
#pragma clang riscv int i = 12;
```
See `HandlePragmaAttribute()` for an example (though you'll look for `eod`
instead of `eof`).
================
Comment at: clang/lib/Parse/ParsePragma.cpp:3971
+ << PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'vector'";
+ return;
+ }
----------------
Same here.
================
Comment at: clang/lib/Parse/ParsePragma.cpp:3978
+ << "clang riscv intrinsic";
+ return;
+ }
----------------
And here as well.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:100
+ switch (Type->getElementBitwidth()) {
+ case 64:
+ QT = Context.DoubleTy;
----------------
I almost hate to ask, but... `long double`? Any of the 16-bit float types?
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:111
+ break;
+ default:
+ llvm_unreachable("Unhandled type.");
----------------
Might as well handle `Invalid` and then drop the `default` entirely so it's a
fully covered switch.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:121
+ // Transform the type to a pointer as the last step, if necessary.
+ if (Type->isPointer())
+ QT = Context.getPointerType(QT);
----------------
Double-checking -- do you have to care about references as well?
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:185-186
+
Record.OverloadedSuffixSize);
+ for (int TypeRangeMaskShift = 0;
+ TypeRangeMaskShift <= static_cast<int>(BasicType::MaxOffset);
+ ++TypeRangeMaskShift) {
----------------
Given that we're bit twiddling with this, I'd feel more comfortable if this was
`unsigned int` rather than `int` (same for `BaseTypeI`).
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:223-225
+ if (!Types.hasValue()) {
+ continue;
+ }
----------------
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:227-229
+ auto SuffixStr =
+ RVVIntrinsic::getSuffixStr(BaseType, Log2LMUL, SuffixProto);
+ auto OverloadedSuffixStr = RVVIntrinsic::getSuffixStr(
----------------
You should spell the type explicitly here instead of using `auto`.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:235-237
+ bool HasMask = Record.MaskedPrototypeLength != 0;
+
+ if (HasMask) {
----------------
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:299-301
+ auto Sigs = IDef.Signature;
+ size_t SigLength = Sigs.size();
+ auto ReturnType = Sigs[0];
----------------
Spell out the types instead of using `auto`.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:322
+ SC_Extern, S.getCurFPFeatures().isFPConstrained(), false,
+ BuiltinFuncType->isFunctionProtoType());
+
----------------
No need to calculate this, we already know.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:328
+ SmallVector<ParmVarDecl *, 8> ParmList;
+ for (unsigned IParm = 0, e = FP->getNumParams(); IParm != e; ++IParm) {
+ ParmVarDecl *Parm =
----------------
Naming style fix.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:342
+ // Setup alias to __builtin_rvv_*
+ auto &IntrinsicII = PP.getIdentifierTable().get(IDef.BuiltinName);
+ RVVIntrinsicDecl->addAttr(
----------------
Spell out type name.
================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:358
+ if (OvIItr != OverloadIntrinsics.end()) {
+ auto OvIntrinsicDef = OvIItr->second;
+ for (auto Index : OvIntrinsicDef.Indexs)
----------------
Spell out the type.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:41
+
+ // Supported type, mask of BasicType
+ unsigned TypeRangeMask;
----------------
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:74
+public:
+ static constexpr unsigned INVALID_INDEX = (unsigned)-1;
+
----------------
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:232-237
+ for (const auto &SemaRecord : SemaRecords) {
+ InsertToSignatureSet(SemaRecord.Prototype);
+ InsertToSignatureSet(SemaRecord.MaskedPrototype);
+ InsertToSignatureSet(SemaRecord.Suffix);
+ InsertToSignatureSet(SemaRecord.OverloadedSuffix);
}
----------------
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:239-240
+
+ for (const auto &Sig : Signatures)
+ insert(Sig);
+}
----------------
Pretty sure you can go with this instead.
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
