[PATCH] D31951: TableGen support for parametrized register class information
Simon Dardis via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 12 04:42:05 PDT 2017
sdardis accepted this revision.
sdardis added a comment.
Adding my LGTM to this, I'm not seeing any major issues. Please see my inline comments, there's a number of changes that can be pulled out of this patch and committed separately as they're formatting changes.
You have two NFC changes in this patch which change commented out or #ifdef 0'd code. Looking at the history of that code suggests they have been unused for over 5 years, and should probably be removed.
Can you follow-up this work by improving the error messages from type inferencing failures? Ideally we should always be highlighting the problematic record.
Finally can you wait a day or two before committing this to allow anyone else to comment?
Thanks for doing this.
Simon
================
Comment at: include/llvm/Target/Target.td:59
+
+// A class representing size/alignment characteristics of a register.
+class RegInfo<int RS, int SS, int SA> {
----------------
"A class representing the register size, spill size and spill alignment in bits of a register."
I think it is worth repeating the fact that the values here measure the size in bits, it hit me when I was testing for MIPS.
================
Comment at: include/llvm/Target/Target.td:66
+
+// The register size/alignment information, parametrized by a hw mode.
+class RegInfoByHwMode<list<HwMode> Ms = [], list<RegInfo> Ts = []>
----------------
parametrized -> parameterized.
================
Comment at: include/llvm/Target/Target.td:208
+ // The register size/alignment information, parametrized by a hw mode.
+ RegInfoByHwMode RegInfos;
----------------
parametrized -> parameterized.
================
Comment at: include/llvm/Target/TargetRegisterInfo.h:323
/// Return the minimum required alignment for a spill slot for a register
/// of this class.
----------------
"Return the minimum required alignment in bytes for a spill slot for a register of this class."
This can be committed separately.
================
Comment at: include/llvm/Target/TargetRegisterInfo.h:338
/// Loop over all of the value types that can be represented by values
// in the given register class.
vt_iterator legalclasstypes_begin(const TargetRegisterClass &RC) const {
----------------
Replace the double slash here with a triple slash.
This can be committed separately.
================
Comment at: test/TableGen/HwModeSelect.td:2
+// RUN: not llvm-tblgen -gen-dag-isel -I %p/../../include %s 2>&1 | FileCheck %s
+
+include "llvm/Target/Target.td"
----------------
Add a single sentence describing the purpose of this test.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:61
-EEVT::TypeSet::TypeSet(ArrayRef<MVT::SimpleValueType> VTList) {
- assert(!VTList.empty() && "empty list?");
- TypeVec.append(VTList.begin(), VTList.end());
-
- if (!VTList.empty())
- assert(VTList[0] != MVT::iAny && VTList[0] != MVT::vAny &&
- VTList[0] != MVT::fAny);
+// This is a parametrized type-set class. For each mode there is a list
+// of types that are currently possible for a given tree node. Type
----------------
parametrized -> parameterized.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:708-711
+/// 1. Ensure that for each type T in "this", there exists a type U in VTS,
+/// such that T and U have equal size in bits.
+/// 2. Ensure that for each type U in VTS, there exists a type T in "this"
+/// such that T and U have equal size in bits (reverse of 1).
----------------
Comment needs updating to match the function declaration.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:1813
- // Check that the number of operands is sane. Negative operands -> varargs.
if (NI.getNumOperands() >= 0 &&
----------------
Restore this comment.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:2103-2109
#if 0
- if (!hasTypeSet() || !getChild(0)->hasTypeSet()) {
+ if (!hasConcreteType() || !getChild(0)->hasConcreteType()) {
bool MadeChange = UpdateNodeType(getChild(0)->getExtType(), TP);
MadeChange |= getChild(0)->UpdateNodeType(getExtType(), TP);
return MadeChange;
}
#endif
----------------
Aside: it appears this code has been commented out since rL98534. I suspect that hunk could be completely removed as a separate commit.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:2537
+ // Break patterns with parametrized types into a series of patterns,
+ // where each one has a fixed type and is predicated on the conditions
----------------
parametrized -> parameterized.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3265-3266
-
-
void CodeGenDAGPatterns::InferInstructionFlags() {
----------------
This can be committed separately.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3807
+
+/// Dependent variable map for CodeGenDAGPattern variant generation
+typedef std::map<std::string, int> DepVarMap;
----------------
Full stop at the end of the sentence.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3820
+/// Find dependent variables within child patterns
+static void FindDepVars(TreePatternNode *N, MultipleUseVarSet &DepVars) {
----------------
Full stop at the end of the sentence.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3831
+#ifndef NDEBUG
+/// Dump the dependent variable set:
+static void DumpDepVars(MultipleUseVarSet &DepVars) {
----------------
: -> .
================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:3832
+/// Dump the dependent variable set:
+static void DumpDepVars(MultipleUseVarSet &DepVars) {
+ if (DepVars.empty()) {
----------------
Requires LLVM_DUMP_METHOD.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:62
+ }
+ MVT getMachineValueType() const {
+ assert(isMachineValueType());
----------------
Whitespace consistency: Add a blank line before this function declaration.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:99-130
+ ValueTypeByHwMode getConcrete(const TypeSetByHwMode &VTS,
+ bool AllowEmpty) const {
+ assert(VTS.isValueTypeByHwMode(AllowEmpty));
+ return VTS.getValueTypeByHwMode();
+ }
+
+ bool MergeInTypeInfo(TypeSetByHwMode &Out, const TypeSetByHwMode &In);
----------------
Can you restore the comments that these functions had and document the new functions?
================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:691
+ Predicate(Record *R, bool C = true) : Def(R), IfCond(C), IsHwMode(false) {
+ assert(R->isSubClassOf("Predicate"));
+ }
----------------
This assert needs an error string.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:697
+ std::string getCondString() const {
+ // The string will excute in a subclass of SelectionDAGISel.
+ // Cast to std::string explicitly to avoid ambiguity with StringRef.
----------------
Return a string which contains the predicates for instruction selection. (or similar).
This comment should be a comment above the function as it describes the function's purpose. The comment below can stay in the function's scope as it's an implementation detail.
================
Comment at: utils/TableGen/CodeGenDAGPatterns.h:776-777
std::map<Record*, SDNodeInfo, LessRecordByID> SDNodes;
- std::map<Record*, std::pair<Record*, std::string>, LessRecordByID> SDNodeXForms;
+ std::map<Record*, std::pair<Record*, std::string>, LessRecordByID>
+ SDNodeXForms;
std::map<Record*, ComplexPattern, LessRecordByID> ComplexPatterns;
----------------
This formatting change can be committed separately.
================
Comment at: utils/TableGen/CodeGenHwModes.cpp:27
+
+void HwMode::dump() const {
+ dbgs() << Name << ": " << Features << '\n';
----------------
Requires LLVM_DUMP_METHOD.
================
Comment at: utils/TableGen/CodeGenHwModes.cpp:46
+
+void HwModeSelect::dump() const {
+ dbgs() << '{';
----------------
Requires LLVM_DUMP_METHOD.
================
Comment at: utils/TableGen/CodeGenHwModes.cpp:82
+ auto F = ModeIds.find(Name);
+ assert(F != ModeIds.end());
+ return F->second;
----------------
Requires an error string.
================
Comment at: utils/TableGen/CodeGenHwModes.cpp:88
+ auto F = ModeSelects.find(R);
+ assert(F != ModeSelects.end());
+ return F->second;
----------------
Requires an error string.
================
Comment at: utils/TableGen/CodeGenHwModes.cpp:92
+
+void CodeGenHwModes::dump() const {
+ dbgs() << "Modes: {\n";
----------------
Requires LLVM_DUM_METHOD.
================
Comment at: utils/TableGen/CodeGenHwModes.h:9
+//===----------------------------------------------------------------------===//
+// Classes to parse and store HW mode information for instruction selection
+//===----------------------------------------------------------------------===//
----------------
Full stop at the end of the sentence.
================
Comment at: utils/TableGen/CodeGenHwModes.h:49
+ const HwMode &getMode(unsigned Id) const {
+ assert(Id != 0);
+ return Modes[Id-1];
----------------
Requires an error string.
================
Comment at: utils/TableGen/DAGISelMatcherGen.cpp:45
+ ValueTypeByHwMode T = RC.getValueTypeNum(0);
+ assert(!T.isSimple() || T.getSimple().SimpleTy == VT);
+#endif
----------------
This requires an error string.
================
Comment at: utils/TableGen/FastISelEmitter.cpp:221-223
// Handle unmatched immediate sizes here.
- //if (Op->getType(0) != VT)
+ //if (Op->getSimpleType(0) != VT)
// return false;
----------------
This has been commented out since rL129691 and can probably be removed in a separate commit.
================
Comment at: utils/TableGen/InfoByHwMode.cpp:9
+//===----------------------------------------------------------------------===//
+// Classes that implement data parametrized by HW modes for instruction
+// selection. Currently it is ValueTypeByHwMode (parametrized ValueType),
----------------
parametrized -> parameterized and the two occurrences below.
================
Comment at: utils/TableGen/InfoByHwMode.cpp:112-115
+#ifndef NDEBUG
+ if (!Rec->isSubClassOf("ValueType"))
+ Rec->dump();
+#endif
----------------
Is this debugging code left over from development?
================
Comment at: utils/TableGen/InfoByHwMode.cpp:116
+#endif
+ assert(Rec->isSubClassOf("ValueType"));
+ if (Rec->isSubClassOf("HwModeSelect"))
----------------
Requires an error string.
================
Comment at: utils/TableGen/InfoByHwMode.h:10
+// Classes that implement data parametrized by HW modes for instruction
+// selection. Currently it is ValueTypeByHwMode (parametrized ValueType),
+// and RegSizeInfoByHwMode (parametrized register/spill size and alignment
----------------
parametrized -> parameterized. And below too.
================
Comment at: utils/TableGen/InfoByHwMode.h:131
+
+
+struct RegSizeInfo {
----------------
Unnecessary blank line.
Repository:
rL LLVM
https://reviews.llvm.org/D31951
More information about the llvm-commits
mailing list