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

[Diana Picus via Phabricator via llvm-commits]

rovka 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();
----------------
Do you really need typename here or is it a leftover?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:513
+
+class CopyRenderer : public OperandRenderer {
+protected:
----------------
This and AddRegisterRenderer could each use a comment (either here or in the RendererKind enum).


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:596
+
+  /// True if the instruction can be build solely by mutating the opcode.
+  bool canMutate() const {
----------------
Typo: built


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:637
+    for (const auto &Renderer : OperandRenderers) {
+      OS << "    ";
+      Renderer->emitCxxRenderStmts(OS);
----------------
Nit: Since emitCxxRenderStmts may emit more than one statement in the general case, I would move this into the function itself and let it deal with all the indentation and newlines that are needed.


================
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);
----------------
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.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:877
+      }
+      return SkipReason{"Dst pattern child isn't a leaf node"};
+    }
----------------
...or an MBB :)


https://reviews.llvm.org/D29712