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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D31135#715631, @ab wrote:

> This seems to only add optional uses, what about OptionalDefOperand?  (I'm fine with "we don't need that", but I don't see anything that would make the import fail)


I think this patch implements OptionalDefOperand but the ISel aspects of OptionalDefOperand are a bit strange. There's three uses of it in LLVM at the moment, all of them in ARM:

- ldstm_mode is never used as far as I can tell and the default operand is '1' which doesn't make sense for an explicit def
- Despite its name, cc_out only ever appears at the end of the input operand list (see the `sI` class) and seems to behave like OperandWithDefaultOps
- s_cc_out is used as an optional def but only appears in InstAlias's



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


================
Comment at: test/TableGen/GlobalISelEmitter.td:242
+
+// CHECK-LABEL: if ([&]() {
+// CHECK-NEXT:    MachineInstr &MI0 = I;
----------------
rovka wrote:
> 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?
There's no requirement that they're unique, it's just that CHECK-LABEL can match in unexpected places if they're not distinct enough. In this case, it's being used to chop up the input into individual rules so that CHECK doesn't match in another rules code although this is moot now that we only have CHECK-NEXT's.

> Maybe we could generate some comment next to each lambda to disambiguate between them?

Now that you mention it, the pattern comment (e.g. line 253) should probably be in front of the 'if ([&]() {' line. I think that's for a different patch though.


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


================
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;
----------------
rovka wrote:
> 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...
Are you thinking of the case where the OperandWithDefaultOps has 2 operands and a Pat<> provides just one? Yes, that would mess up the algorithm. I don't think anyone has used OperandWithDefaultOps like that but I should at least check for it so we fail nicely if someone has done that.


================
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;
----------------
rovka wrote:
> 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.
Ok


https://reviews.llvm.org/D31135