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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab marked an inline comment as done.
ab added a comment.

Thanks for the reviews, all!



================
Comment at: include/llvm/Target/TargetGlobalISel.td:23
+  Instruction I = i;
+}
+
----------------
qcolombet wrote:
> dsanders wrote:
> > qcolombet wrote:
> > > 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.
> > We haven't used it yet so I can't be sure but G_SEQUENCE seems like the gMIR equivalent of BUILD_VECTOR. Is that right?
> Yep, that's a poor example. SEXTLOAD is probably a better one. 
Eventually we'll need to support many-to-1 or 1-to-many patterns (as Daniel mentioned), but those should still map, at their core, nodes to instructions.   When we get to the point where we need to map one SDNode to multiple instructions (or the opposite), it will probably be time to switch to a first-class GISel representation!  (and it's always possible to introduce new pseudo instructions to replace the target-specific ISD opcodes.)


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:99
+
+struct MatchAction {
+  virtual ~MatchAction() {}
----------------
dsanders wrote:
> ab wrote:
> > dsanders wrote:
> > > This is a nitpick but I think we should keep Matchers and Actions in separate class hierarchies for type checking purposes. They don't share much implementation and it doesn't make sense to put MatchOpcode in Matcher::Actions or MutateOpcode in Matcher::Matchers
> > Fair enough; split it into 'Matcher' and 'MatchAction', and renamed 'Matcher' to 'MatcherEmitter'.  Better names welcome ;)
> For 'Matcher', could you elaborate that it's matching properties of an operand (OperandPropertyMatcher?). That way we won't have to rename it when we add additional levels of matching. For example, I've added an InstructionMatcher in my current WIP and I suspect I'll find an OperandMatcher useful now that we can have multiple matchers on a single operand.
What about opcode matching though?  Are you thinking of a hierarchy of specialized Matchers ?  I can see it being useful, but I wouldn't worry about renaming everything once we get there ;)


================
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) {
----------------
qcolombet wrote:
> 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.
LLT alone isn't always compatible with MVT: things like iPTR require a DataLayout;  the various 'iAny' types need extra logic, and then there are types that just don't have LLT equivalents (e.g., glue, or x86mmx today).

Added an explanation.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:94
+static bool isTrivialOperatorNode(const TreePatternNode *N) {
+  return !N->isLeaf() && !N->hasAnyPredicate() && !N->getTransformFn();
+}
----------------
qcolombet wrote:
> 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.
Good point;  physregs that are "set" would be rejected when we look for RegisterClasses, but "implicit" defs are still a possibility.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:344
+  // Look through the SelectionDAG patterns we found, possibly emitting some.
+  for (const PatternToMatch &Pat : CGP.ptms()) {
+    ++NumPatternTotal;
----------------
zvi wrote:
> To the best of my knowledge there is no CodeGenDAGPatterns::ptms().
Good eye, separate commit in my local branch!  I trimmed and squashed it when committing though, I didn't end up needing all the iterators and ranges I added.


Repository:
  rL LLVM

https://reviews.llvm.org/D26878