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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

> 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.

> 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.

All in all, the patch looks sensible to me.



================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1240
+  unsigned ShVal = AArch64_AM::getShifterImm(AArch64_AM::LSL, ShiftAmt);
+  Result1 = MachineOperand::CreateImm(Immed);
+  Result2 = MachineOperand::CreateImm(ShVal);
----------------
dsanders wrote:
> qcolombet wrote:
> > dsanders wrote:
> > > qcolombet wrote:
> > > > Can't we modify the existing placeholder instead of creating a temporary object and then use the copy constructor?
> > > I think we should be able to use ChangeToImmediate(), but I haven't tried it yet. The bit that worried me about doing that was having stale values in members that aren't affected by the ChangeTo*() functions. Most of them look ok (since they only matter for registers and ChangeToRegister() does reset them) but I'm not sure about MachineOperand::ParentMI.
> > That should be easy to check/fix :).
> When we create new operands, the ChangeTo*() family are ok so long as you also call clearParent() afterwards to clear and parents left over from previous predicates.
I would have expected this to be handled by the ChangeTo* APIs.
There might be a good reason why this is not the case. Do you know?


https://reviews.llvm.org/D30089