craig.topper added inline comments.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:70
+ // passing to the BUILTIN() macro in Builtins.def.
+ const std::string &builtin_str() const { return BuiltinStr; }
+
----------------
These method names should use CamelCase and start with "get"
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:102
+ // Compute and record a string for legal type.
+ void compute_builtin_str();
+ // Compute and record a builtin RVV vector type string.
----------------
These should use CamelCase per llvm coding style.
Might be better named init*Str instead of compute. compute makes me think they
are going to return something, but that might just be me.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:267
+RVVType::RVVType(BasicType BT, int Log2LMUL, StringRef prototype)
+ : BT(BT), LMUL(LMULType(Log2LMUL)), IsFloat(false), IsBool(false),
+ IsSigned(true), IsImmediate(false), IsVoid(false), IsConstant(false),
----------------
You can initialize the bools to false with " = false" where they are declared
in the class body then you don't need to mention them all here. Similar for
Scale and ElementBitWidth.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:270
+ IsPointer(false), IsSize_t(false), IsPtrdiff_t(false),
+ ElementBitwidth(~0U), Scale(0) {
+ applyBasicType();
----------------
Why is ElementBitwidth default ~0. Wouldn't 0 also be an invalid value?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:333
+ return;
+ } else if (IsSize_t) {
+ S = "z";
----------------
You drop the else since the if above returned.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:618
+ if (NewMangledName.empty())
+ MangledName = Twine(NewName.split("_").first).str();
+ else
----------------
I don't think we need to go through Twine here. We should be able to call str()
directly on first.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:621
+ MangledName = NewMangledName.str();
+ if (Suffix.size())
+ Name += "_" + Suffix.str();
----------------
!Suffix.empty()
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:640
+ InputTypes.assign(OutInTypes.begin() + 1, OutInTypes.end());
+ for (unsigned i = 0; i < InputTypes.size(); ++i)
+ CTypeOrder.push_back(i);
----------------
CTypeOrder.resize(InputTypes.size());
std::iota(CTypeOrder.begin(), CTypeOrder.end(), 0);
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:710
+void RVVIntrinsic::emitMangledFuncDef(raw_ostream &OS) const {
+ OS << Twine(OutputType->type_str() + Twine(" ")).str();
+ OS << getMangledName();
----------------
Can't we just print OutputType->type_str() and " " to OS separately? We
shouldn't need to concat them into a Twine first.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:714
+ // Emit function arguments
+ if (CTypeOrder.size() > 1) {
+ OS << InputTypes[CTypeOrder[0]]->type_str() + " op0";
----------------
Why is this > 1 and not >= 1 or !CTypeOrder.empty()?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:720
+ OS << "){\n";
+ OS << " return " + getName() + "(";
+ // Emit parameter variables
----------------
Replace the + operators with <<
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:722
+ // Emit parameter variables
+ if (CTypeOrder.size() > 1) {
+ OS << "op0";
----------------
Same here, why is this >1 and not >=1 or !CTypeOrder.empty()?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:749
+
+ OS << "#ifndef _RISCV_VECTOR_H\n";
+ OS << "#define _RISCV_VECTOR_H\n\n";
----------------
Looks like other headers use 2 underscores at the beginning of their include
guard.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:757
+ OS << "#error \"Vector intrinsics require the vector extension.\"\n";
+ OS << "#else\n\n";
+
----------------
Can we just #endif here instead of the #else? If the error is emitted the
preprocessor should stop and not process the rest of the file. Then we don't
need to close it at the bottom of the file.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:767
+ // Dump RVV boolean types.
+ auto dumpType = [&](auto T) {
+ OS << "typedef " << T->clang_builtin_str() << " " << T->type_str() <<
";\n";
----------------
I'd recommend calling this printType. dump made me think it was printing for
debug like the dump() functions found in many LLVM classes.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:799
+ // D implies F
+ OS << "#if defined(__riscv_f) || defined(__riscv_d)\n";
+ for (int Log2LMUL : Log2LMULs) {
----------------
I think we only need to check __riscv_f here?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:836
+ return;
+ OS << StringRef(
+ "static inline __attribute__((__always_inline__, __nodebug__, "
----------------
I don't think this StringRef constructor call is needed.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:849
+ OS << "#define RISCVV_BUILTIN(ID, TYPE, ATTRS) BUILTIN(ID, TYPE, ATTRS)\n";
+ OS << "#endif\n";
+ for (auto &Def : Defs) {
----------------
We probably want 2 newlines on the end of this so there will be a blank line
before the first RISCVV_BUILTIN.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:873
+ // Dump switch body when the ir name changes from previous iteration.
+ RVVIntrinsic *PrevDef = Defs.begin()->get();
+ for (auto &Def : Defs) {
----------------
Can we remember the PrevIRName StringRef instead?
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:883
+ }
+ (*Defs.rbegin())->emitCodeGenSwitchBody(OS);
+ OS << "break;\n";
----------------
Defs.back()->emitCodeGenSwitchBody(OS);
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1027
+ return false;
+ bool NeedOR = false;
+ OS << "#if";
----------------
I think you can use ListSeparator for this. It keeps track of the separator
string and whether the first item has been printed. It's most often used with
loops, but it should work here. I think there are many examples uses in
llvm/utils/TableGen
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95016/new/
https://reviews.llvm.org/D95016
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
