[PATCH] D31951: TableGen support for parametrized register class information

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

Hi Krzysztof,

I haven't forgotten about this patch, I'm currently on and off looking at this and testing it for MIPS.

Another round of comments.

Thanks,
Simon



================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:235-237
+void VariableTypeSet::dump() const {
+  dbgs() << getAsString() << '\n';
+}
----------------
Mark this method as LLVM_DUMP_METHOD - see include/llvm/Support/Compiler.h.


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:292
+  assert(!AllEmpty &&
+          "type set is empty for each hw mode: type contradition?");
+#endif
----------------
contradition -> contradiction.


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:620
 
-  return MadeChange;
-}
+  auto NoSupV = [&IsSubVec](const VariableTypeSet::SetType &S, MVT T) -> bool {
+    for (const auto &I : S)
----------------
Comment needed here.


================
Comment at: utils/TableGen/CodeGenHwModes.cpp:1
+#include "CodeGenHwModes.h"
+#include "llvm/Support/Debug.h"
----------------
sdardis wrote:
> Requires LLVM's standard header.
C++ tag required here too.


================
Comment at: utils/TableGen/CodeGenHwModes.h:1
+#ifndef LLVM_UTILS_TABLEGEN_CODEGENHWMODES_H
+#define LLVM_UTILS_TABLEGEN_CODEGENHWMODES_H
----------------
sdardis wrote:
> Requires LLVM's standard header.
Requires C++ tag.


================
Comment at: utils/TableGen/CodeGenRegisters.cpp:728
   unsigned Size = R->getValueAsInt("Size");
+  assert(VRI.hasDefault() || Size != 0 || VTs[0].isSimple());
+  if (!VRI.hasDefault()) {
----------------
Requires an error string.


================
Comment at: utils/TableGen/FastISelEmitter.cpp:164
+      vi::VariableValueType VVT = TP->getTree(0)->getType(0);
+      assert(VVT.isSimple());
+      OS << "VT == " << getEnumName(VVT.getSimple().SimpleTy) << " && ";
----------------
Error string required here.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:1209-1210
   // Emit SubRegIndex lane masks, including 0.
-  OS << "\nstatic const LaneBitmask SubRegIndexLaneMaskTable[] = {\n  LaneBitmask::getAll(),\n";
+  OS << "\nstatic const LaneBitmask SubRegIndexLaneMaskTable[] = {\n  "
+        "LaneBitmask::getAll(),\n";
   for (const auto &Idx : SubRegIndices) {
----------------
Can you commit this separately?


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:1274-1275
     for (const auto &RC : RegisterClasses) {
-      OS << "static const uint32_t " << RC.getName() << "SubClassMask[] = {\n  ";
+      OS << "static const uint32_t " << RC.getName()
+         << "SubClassMask[] = {\n  ";
       printBitVectorAsHex(OS, RC.getSubClasses(), 32);
----------------
Can you commit this separately?


================
Comment at: utils/TableGen/VariableValueType.cpp:40
+bool VariableValueType::operator== (const VariableValueType &T) const {
+  assert(isValid() && T.isValid());
+  bool Simple = isSimple();
----------------
Needs an error string.


================
Comment at: utils/TableGen/VariableValueType.cpp:51
+bool VariableValueType::operator< (const VariableValueType &T) const {
+  assert(isValid() && T.isValid());
+  // Default order for maps.
----------------
Needs an error string.


================
Comment at: utils/TableGen/VariableValueType.cpp:98
+
+void VariableValueType::dump() const {
+  dbgs() << "size=" << Map.size() << '\n';
----------------
Needs standard LLVM_DUMP_METHOD wrapping.


================
Comment at: utils/TableGen/VariableValueType.h:9
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_UTILS_TABLEGEN_VARIABLEVALUETYPE_H
----------------
Needs documentation describing purpose.


Repository:
  rL LLVM

https://reviews.llvm.org/D31951