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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Sorry for the slow reply on this one.

In https://reviews.llvm.org/D30089#680485, @qcolombet wrote:

> One remark: I can see how a placeholder operand can help detecting errors (like we forgot to update one of the operand), however, I think it gets in the way of efficiency.
>  Indeed, I imagine that each time a complex pattern will fail half way through, we will have to reset all the operands to placeholder instead of just keeping them in the state they are.


It's not really aimed at detecting errors but rather at constructing the MachineOperand without creating fake data to put in it. Each successful ComplexPattern predicate will fully overwrite the placeholder (using the default copy constructor) so I don't think we need to reset it between rules. I can see how it would be useful to reset it to the placeholder data to ensure the predicate is behaving correctly though since if a predicate returns true but the placeholder data is still there then there's a bug in the predicate.

One other thing to mention is that I'd like to skip the constructor call completely and leave it uninitialized until the first successful ComplexPattern predicate overwrites it. Initializing the operands to a placeholder is just unnecessary overhead.

> What I am saying is that we may not introduce a placeholder operand at all. We should have a placeholder opcode though that reset the operands array to 0 (without freeing memory).

What would be the benefit of a placeholder instruction over placeholder operands? I think it might be slightly higher cost since if we reset the operand list between rules then it increases the number of MachineOperand constructor calls (but not allocations since it uses the placement new operator). A placeholder instruction also means the instruction and operands are heap allocated instead of stack allocated and will therefore cause a memory leak unless additional overhead to free the instruction is added.



================
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);
----------------
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.


https://reviews.llvm.org/D30089