[PATCH] D21726: CodeGen: Use MachineInstr& in TargetInstrInfo, NFC

[Justin Bogner via llvm-commits]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>> On 2016-Jun-29, at 09:45, Justin Bogner <mail at justinbogner.com> wrote:
>> 
>> "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>>> dexonsmith created this revision.
>>> 
>>> This is mostly a mechanical change to make TargetInstrInfo API take
>>> MachineInstr& (instead of MachineInstr* or MachineBasicBlock::iterator)
>>> when the argument is expected to be a valid MachineInstr.  This is a
>>> general API improvement.
>> ...
>>> dexonsmith updated this revision to Diff 62164.
>>> dexonsmith added a comment.
>>> 
>>> - Spell NULL as nullptr in moved comment, and clean up the
>>> areCFlagsAccessedBetweenInstrs helper in AArch64InstrInfo.cpp
>>> (responding to Ahmed's feedback).
>>> - Update off-by-default targets.
 ...
>>>     switch (Imm) {
>>>     case 0x00: // EQUAL
>>>     case 0x03: // UNORDERED
>>>     case 0x04: // NOT EQUAL
>>>     case 0x07: // ORDERED
>>> -      if (NewMI) {
>>> -        MachineFunction &MF = *MI->getParent()->getParent();
>>> -        MI = MF.CloneMachineInstr(MI);
>>> -        NewMI = false;
>>> -      }
>>>       return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> 
>> Shouldn't this follow the pattern in the rest of this function, and do
>> something like so?
>> 
>>    auto &WorkingMI = cloneIfNew(MI);
>>    return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false);
>
> These instructions want exactly the base class behaviour from
> TargetInstrInfo::commuteInstructionImpl.  I think it's simpler to
> forward directly without filtering the parameters.  However, I can see
> the argument to make this path consistent with the rest of
> X86InstrInfo::commuteInstructionImpl.
>
> I'm happy either way.  Let me know if I haven't convinced you and I'll swap it.

Sure, this makes sense. It's less strange when looking at it in tree
rather than in the context of the patch, anyways.

This all LGTM now, go ahead and commit at your convenience.


As an aside, there is a bit of an API issue I noticed because of this
change. BuildMI has overloads for MachineInstr*, MBB::iterator, and
MBB::instr_iterator, but not MachineInstr&. This actually means that if
someone changes a caller to pass a MachineInstr& instead of a
MachineInstr*, they'll get a behaviour change and potentially introduce
a bug. This didn't come up in this patch though, since it always changed
from an iterator to a reference. Maybe we should add a MachineInstr&
overload, just to be safe?