[PATCH] D29712: [globalisel] Decouple src pattern operands from dst pattern operands.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 2 inline comments as done.
dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:449
+  unsigned getNumOperands() const { return Operands.size(); }
+  typename OperandVec::const_iterator operands_begin() const {
+    return Operands.begin();
----------------
rovka wrote:
> Do you really need typename here or is it a leftover?
It's a copy/paste from predicates_begin(). It's needed there but isn't here.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:863
+  // Finally render the used operands (i.e., the children of the root operator).
+  for (unsigned i = 0, e = Dst->getNumChildren(); i != e; ++i) {
+    auto *DstChild = Dst->getChild(i);
----------------
rovka wrote:
> This is very copy-pasted from above, would it make sense to extract it into, say, a processChildren() taking a Handler of some sort? Only if you think it won't make things more difficult in the future, ofc.
The reason for separating the two is that the source pattern and destination pattern have different jobs and support different kinds of DAG. It's also possible for the two patterns to be very different in structure. For example:
   (add (mul $src1:GPR32, $src2:GPR32), $src3:GPR32)
  =>
   (SMADDLrrr $src1:GPR32, $src2:GPR32, $src3:GPR32)

It might be possible to turn it into a processChildren() + handler setup. I'm going to be trying to turn this into a proper tree walker soon to support nested instructions.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:877
+      }
+      return SkipReason{"Dst pattern child isn't a leaf node"};
+    }
----------------
rovka wrote:
> ...or an MBB :)
Thanks. I should have changed that before splitting the loop into two :-)


https://reviews.llvm.org/D29712