barannikov88 added inline comments.
================ Comment at: llvm/lib/TargetParser/CMakeLists.txt:29 +# LLVMTargetParser. See https://stackoverflow.com/a/25681179 +target_include_directories(LLVMTargetParser PUBLIC $<BUILD_INTERFACE:${LLVM_LIBRARY_DIR}/TargetParser/>) ---------------- Will it work if RISC-V target is not compiled-in? This does not strictly add a cyclic dependency, but it would still be nice to avoid dependency on higher-level components. Is it possible / reasonable to extract the part of the RISCV.td that relates to this component and put it separate td file in this directory? Or is it tightly coupled with the rest of the target description? ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:15 +#include "llvm/TableGen/Record.h" +#include <iostream> +namespace llvm { ---------------- <iostream> [[ https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden | is forbidden ]] by the coding standards. ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:16 +#include <iostream> +namespace llvm { + ---------------- This [[ https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions | should be ]] `using namespace llvm;` ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:18 + +std::string getEnumFeatures(Record &Rec) { + std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features"); ---------------- This should be `static`. ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:20 + std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features"); + if (std::find_if(Features.begin(), Features.end(), [](Record *R) { + return R->getName() == "Feature64Bit"; ---------------- (suggestion) LLVM's version of find_if accepts ranges, which is a bit shorter. ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:28 + +void EmitRISCVTargetDef(RecordKeeper &RK, raw_ostream &OS) { + const auto &Map = RK.getDefs(); ---------------- This function does not seem to mutate RecordKeeper, so it should be `const`. ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:37 + // Iterate on all definition records. + for (auto &Def : Map) { + const auto &Record = Def.second; ---------------- Should also `const`, same for the loop below. ================ Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:52 + for (auto &Def : Map) { + const auto &Record = Def.second; + if (Record->isSubClassOf("RISCVProcessorModelTUNE_PROC")) ---------------- Same for the loop above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137517/new/ https://reviews.llvm.org/D137517 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
