[PATCH] D30089: [globalisel][tblgen] Add support for ComplexPatterns

[Quentin Colombet via llvm-commits]

> On Mar 10, 2017, at 3:43 PM, Daniel Sanders via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> dsanders added a comment.
> 
> In https://reviews.llvm.org/D30089#698324, @qcolombet wrote:
> 
>>> The constructor call comes from the need to call addOperand(). If we reset the operand list for the placeholder instruction on each call to selectImpl() then we would need something like:
>>> 
>>> PlaceholderInsn.addOperand(MachineOperand::CreatePlaceholder());
>>> PlaceholderInsn.addOperand(MachineOperand::CreatePlaceholder());
>>> if (matches && selectComplexPattern(PlaceholderInsn.getOperand(0), PlaceholderInsn.getOperand(1))) {
>>> 
>>>  BuildMI(...).add(PlaceholderInsn.getOperand(0)).add(PlaceholderInsn.getOperand(1));
>>>  ...
>>> 
>>> }
>>> PlaceholderInsn.resetOperands();
>>> Each call to addOperand() involves a placement new operator which uses the copy-constructor and therefore pays the same overhead as the current local variable approach.
>> 
>> I was actually expecting more something like:
>> PlaceHolderInst.setNumOperand(NbOp);
>> With that call just allocating/resizing the operand array accordingly. The operands created could be left uninitialized as they are supposed to be all properly set by the matching.
> 
> 
> Ah ok, I see how that would avoid the redundant initializations. Wouldn't it be more efficient to call setNumOperands(MaxNumOperands) in advance though to save on re-allocating and copying the array?

Exactly.

> 
>>> I think your main concern is that the overhead is unnecessarily paid on each call to selectImpl(). I've updated the patch with a version that only pays the constructor overhead once (during <Target>InstructionSelection's constructor). Does that resolve your concerns?
>> 
>> Yes, partly. I believe we don't need the OperandHolder all together, but this is an optimization that we definitely don't need to do now.
> 
> I'm not sure we can eliminate the placeholders since storing directly to the output instruction requires us to build that instruction before we've finished matching but I'll have a think on how we can do it.

The way I was seeing the placeholder is that it would be created by the caller (InstructionSelect pass) and reused for all patterns until a matching work. Then the placeholder is mutated by the matching and the caller creates a new placeholder only now.

Again we are talking optimization, I’d rather have something working first :).

> 
>> All in all, the patch looks sensible to me.
> 
> Thanks.
> 
> 
> https://reviews.llvm.org/D30089
> 
> 
>