[PATCH] D31135: [globalisel][tablegen] Add experimental support for OperandWithDefaultOps, PredicateOperand, and OptionalDefOperand

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: test/TableGen/GlobalISelEmitter.td:29
+def m1 : OperandWithDefaultOps <i32, (ops (i32 -1))>;
+def Z : OperandWithDefaultOps <i32, (ops R0)>;
+
----------------
Could you also add a test with 2 ops for the same operand? That would correspond to ARM's predicate operand.


================
Comment at: test/TableGen/GlobalISelEmitter.td:242
+
+// CHECK-LABEL: if ([&]() {
+// CHECK-NEXT:    MachineInstr &MI0 = I;
----------------
Sorry I didn't notice this before, but I'm not sure this is a good use for CHECK-LABEL. IIUC the labels are supposed to be unique within the file. I might be wrong about it, but I thought I should bring it up just in case... Maybe we could generate some comment next to each lambda to disambiguate between them?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:36
 #include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/CodeGen/MachineValueType.h"
----------------
Nit: This is not properly ordered (should go above Statistic.h)


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1436
+  // OperandWithDefaultOps are considered from left to right until we have
+  // enough operands to render the instruction.
+  SmallSet<unsigned, 1> DefaultOperands;
----------------
I'm a bit nervous about what happens if we *do* have a value for one of the default operands. Won't that mess up the whole algorithm? AFAICT we're not bailing out if we end up with too many operands...


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1437
+  // enough operands to render the instruction.
+  SmallSet<unsigned, 1> DefaultOperands;
+  unsigned DstINumUses = DstI.Operands.size() - DstI.Operands.NumDefs;
----------------
Nit: ARM has lots of instructions with 2 default operands (the pred and the condition code). I think it wouldn't hurt much to use 2 here to avoid going to the heap in those cases.


https://reviews.llvm.org/D31135