[PATCH] D31761: [globalisel][tablegen] Revise API for ComplexPattern operands to improve flexibility.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

I'm slightly worried that this makes the selector functions a bit harder to read, but this seems like a nice simplification otherwise.

One question: does this prevent us from ever selecting operands in-place? The previous implementation seems amenable to that (by only assigning the complex-pattern MO instead of adding them all to a new MI).  But now we'd be forced to always append operands, right?

This also means that we can remove MO_Placeholder, yes?



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:80
+  typedef std::function<void(MachineInstrBuilder &)> ComplexRendererFn;
+  typedef Optional<ComplexRendererFn> OptionalComplexRendererFn;
+
----------------
The Optional seems a bit redundant: std::function is already optional.  Can we return a {} default?


================
Comment at: test/TableGen/GlobalISelEmitter-GIRule.td:70
 // CHECK-NEXT:        ((/* src2 */ (MRI.getType(MI0.getOperand(2).getReg()) == (LLT::scalar(32))) &&
-// CHECK-NEXT:        (selectComplexPattern(MI0.getOperand(2), TempOp0, TempOp1)))) &&
+// CHECK-NEXT:        ((Renderer0 = selectComplexPattern(MI0.getOperand(2)), Renderer0.hasValue())))) &&
 // CHECK-NEXT:        ((/* src3 */ (MRI.getType(MI0.getOperand(3).getReg()) == (LLT::scalar(32))) &&
----------------
What about:

        ((Renderer0 = selectComplexPattern(MI0.getOperand(2))))

I found the ',' more surprising than the '='.


https://reviews.llvm.org/D31761