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

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

Hi Krzysztof,

I've gone over this patch and I am in broad agreement with it.

My biggest concern is how we need to have HwModes and ValueTypes & RegInfos as two equally long lists, we should have compile time checks for this.

The nits I see are assertions without error strings, new files lacking the standard header, also the error messages for type inferencing.

I will examine this patch more over the next week or two.

Thanks for doing this,
Simon



================
Comment at: include/llvm/Target/Target.td:55
+    : HwModeSelect<Ms>, ValueType<-1,-1> {
+  // The length of this list must be the same as the length of Ms.
+  list<ValueType> Objects = Ts;
----------------
See my comment on utils/TableGen/CodeGenHwModes.cpp about how I believe we should enforce this.


================
Comment at: include/llvm/Target/Target.td:69
+    : HwModeSelect<Ms> {
+  // The length of this list must be the same as the length of Ms.
+  list<RegInfo> Objects = Ts;
----------------
Likewise.


================
Comment at: include/llvm/Target/Target.td:208
 
+  // The parametrized register size/alignment information.
+  VariableRegInfo VRI;
----------------
Tinsy nit: I think you should reuse the comment from line 66 here, or work "hw mode" into the comment.


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:85
+vi::VariableValueType VariableTypeSet::getVariableValueType() const {
+  assert(isVariableValueType(true));
 
----------------
This needs an error string.


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:295
+    AllEmpty &= I.second.empty();
+  assert(!AllEmpty);
+#endif
----------------
Needs an error string.


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:938-946
+static std::string getPredicateCheck(const std::vector<Predicate> &Preds) {
+  std::string Check;
+  for (unsigned i = 0, e = Preds.size(); i != e; ++i) {
+    if (i != 0)
+      Check += " && ";
+    Check += '(' + Preds[i].getCondString() + ')';
+  }
----------------
Should we ensure the old constraint that the check string has some notion of a canonical order?


================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:682
 
+class Predicate {
+public:
----------------
This could do with a comment documenting it's purpose.


================
Comment at: utils/TableGen/CodeGenHwModes.cpp:1
+#include "CodeGenHwModes.h"
+#include "llvm/Support/Debug.h"
----------------
Requires LLVM's standard header.


================
Comment at: utils/TableGen/CodeGenHwModes.cpp:22-23
+  std::vector<Record*> Objects = R->getValueAsListOfDefs("Objects");
+  assert(Modes.size() == Objects.size() &&
+         "The lists Modes and Objects should have the same size");
+  for (unsigned i = 0, e = Modes.size(); i != e; ++i) {
----------------
I think this check should be promoted to report_fatal_error--rather than an assert--with a location of the problematic record if possible.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D31951