[PATCH] D26878: [GlobalISel] Add tentative Selector-emitted tablegen backend.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Hi Ahmed,

Looks good to me.

Nitpicks below.

Cheers,
-Quentin



================
Comment at: include/llvm/Target/TargetGlobalISel.td:22
+  SDNode Node = node;
+  Instruction I = i;
+}
----------------
Nitpick: Just a personal thing, but given the argument are i then node, I would have expected I then Node.


================
Comment at: include/llvm/Target/TargetGlobalISel.td:23
+  Instruction I = i;
+}
+
----------------
Just a forward looking thought. Do you think that we would need to support 1 to many patterns?
E.g., what will be the strategy for BUILD_VECTOR and the kind?

I am starting to think that we are defining patterns, just target independent ones.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.h:35
 private:
+  bool selectImpl(MachineInstr &I) const;
+
----------------
Could you document how that selectImpl is different from select and how they interact between each other?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:10
+//
+// This tablegen backend emits code for use by the GlobalISel instruction
+// selector. See include/llvm/CodeGen/TargetGlobalISel.td.
----------------
/// Doxygen style?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:16
+// SelectionDAG-specific constructs to their GlobalISel (when applicable: MVT
+// to LLT;  SDNode to generic Instruction).
+//
----------------
There is a word missing "[...] their GlobalISel _counterpart_".


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:16
+// SelectionDAG-specific constructs to their GlobalISel (when applicable: MVT
+// to LLT;  SDNode to generic Instruction).
+//
----------------
qcolombet wrote:
> There is a word missing "[...] their GlobalISel _counterpart_".
I would add comment explaining how to get the list of patterns that are not supported and interpret why they are not supported.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:76
+
+/// Convert an MVT to an equivalent LLT if possible, or the invalid LLT().
+static std::string MVTToLLT(MVT::SimpleValueType SVT) {
----------------
What does it mean for the consumer of that function to have an invalid LLT?

Put differently, how can we get an invalid LLT? I would expect LLT to be a strict superset of MVT.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:94
+static bool isTrivialOperatorNode(const TreePatternNode *N) {
+  return !N->isLeaf() && !N->hasAnyPredicate() && !N->getTransformFn();
+}
----------------
Shouldn't we rule out the patterns with physical registers as well?
Indeed, unless I am mistaken, we don't have emitter to add the implicit def/use for those operands.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:308
+
+    // Otherwise, we're looking for a bog-standard RegisterClass operand.
+    if (SrcChild->hasAnyPredicate())
----------------
bog?


https://reviews.llvm.org/D26878