[PATCH] D21726: CodeGen: Use MachineInstr& in TargetInstrInfo, NFC
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 29 11:46:15 PDT 2016
> 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.
>>
>>
>> http://reviews.llvm.org/D21726
>>
>> Index: lib/Target/X86/X86InstrInfo.cpp
>> ===================================================================
>> --- lib/Target/X86/X86InstrInfo.cpp
>> +++ lib/Target/X86/X86InstrInfo.cpp
> ...
>> -MachineInstr *X86InstrInfo::commuteInstructionImpl(MachineInstr *MI,
>> - bool NewMI,
>> +MachineInstr *X86InstrInfo::commuteInstructionImpl(MachineInstr &MI, bool NewMI,
>> unsigned OpIdx1,
>> unsigned OpIdx2) const {
>> - switch (MI->getOpcode()) {
>> + auto cloneIfNew = [NewMI](MachineInstr &MI) -> MachineInstr & {
>> + if (NewMI)
>> + return *MI.getParent()->getParent()->CloneMachineInstr(&MI);
>> + return MI;
>> + };
>> +
>> + switch (MI.getOpcode()) {
>> case X86::SHRD16rri8: // A = SHRD16rri8 B, C, I -> A = SHLD16rri8 C, B, (16-I)
>> case X86::SHLD16rri8: // A = SHLD16rri8 B, C, I -> A = SHRD16rri8 C, B, (16-I)
>> case X86::SHRD32rri8: // A = SHRD32rri8 B, C, I -> A = SHLD32rri8 C, B, (32-I)
>> case X86::SHLD32rri8: // A = SHLD32rri8 B, C, I -> A = SHRD32rri8 C, B, (32-I)
>> case X86::SHRD64rri8: // A = SHRD64rri8 B, C, I -> A = SHLD64rri8 C, B, (64-I)
>> case X86::SHLD64rri8:{// A = SHLD64rri8 B, C, I -> A = SHRD64rri8 C, B, (64-I)
>> unsigned Opc;
>> unsigned Size;
>> - switch (MI->getOpcode()) {
>> + switch (MI.getOpcode()) {
>> default: llvm_unreachable("Unreachable!");
>> case X86::SHRD16rri8: Size = 16; Opc = X86::SHLD16rri8; break;
>> case X86::SHLD16rri8: Size = 16; Opc = X86::SHRD16rri8; break;
>> case X86::SHRD32rri8: Size = 32; Opc = X86::SHLD32rri8; break;
>> case X86::SHLD32rri8: Size = 32; Opc = X86::SHRD32rri8; break;
>> case X86::SHRD64rri8: Size = 64; Opc = X86::SHLD64rri8; break;
>> case X86::SHLD64rri8: Size = 64; Opc = X86::SHRD64rri8; break;
>> }
>> - unsigned Amt = MI->getOperand(3).getImm();
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->setDesc(get(Opc));
>> - MI->getOperand(3).setImm(Size-Amt);
>> - return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> + unsigned Amt = MI.getOperand(3).getImm();
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.setDesc(get(Opc));
>> + WorkingMI.getOperand(3).setImm(Size - Amt);
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> case X86::BLENDPDrri:
>> case X86::BLENDPSrri:
>> @@ -3212,7 +3229,7 @@
>> case X86::VPBLENDDYrri:
>> case X86::VPBLENDWYrri:{
>> unsigned Mask;
>> - switch (MI->getOpcode()) {
>> + switch (MI.getOpcode()) {
>> default: llvm_unreachable("Unreachable!");
>> case X86::BLENDPDrri: Mask = 0x03; break;
>> case X86::BLENDPSrri: Mask = 0x0F; break;
>> @@ -3227,29 +3244,23 @@
>> case X86::VPBLENDWYrri: Mask = 0xFF; break;
>> }
>> // Only the least significant bits of Imm are used.
>> - unsigned Imm = MI->getOperand(3).getImm() & Mask;
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->getOperand(3).setImm(Mask ^ Imm);
>> - return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> + unsigned Imm = MI.getOperand(3).getImm() & Mask;
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.getOperand(3).setImm(Mask ^ Imm);
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> case X86::PCLMULQDQrr:
>> case X86::VPCLMULQDQrr:{
>> // SRC1 64bits = Imm[0] ? SRC1[127:64] : SRC1[63:0]
>> // SRC2 64bits = Imm[4] ? SRC2[127:64] : SRC2[63:0]
>> - unsigned Imm = MI->getOperand(3).getImm();
>> + unsigned Imm = MI.getOperand(3).getImm();
>> unsigned Src1Hi = Imm & 0x01;
>> unsigned Src2Hi = Imm & 0x10;
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->getOperand(3).setImm((Src1Hi << 4) | (Src2Hi >> 4));
>> - return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.getOperand(3).setImm((Src1Hi << 4) | (Src2Hi >> 4));
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> case X86::CMPPDrri:
>> case X86::CMPPSrri:
>> @@ -3259,17 +3270,12 @@
>> case X86::VCMPPSYrri: {
>> // Float comparison can be safely commuted for
>> // Ordered/Unordered/Equal/NotEqual tests
>> - unsigned Imm = MI->getOperand(3).getImm() & 0x7;
>> + unsigned Imm = MI.getOperand(3).getImm() & 0x7;
>> 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.
>
>> default:
>> return nullptr;
>> @@ -3280,7 +3286,7 @@
>> case X86::VPCOMQri: case X86::VPCOMUQri:
>> case X86::VPCOMWri: case X86::VPCOMUWri: {
>> // Flip comparison mode immediate (if necessary).
>> - unsigned Imm = MI->getOperand(3).getImm() & 0x7;
>> + unsigned Imm = MI.getOperand(3).getImm() & 0x7;
>> switch (Imm) {
>> case 0x00: Imm = 0x02; break; // LT -> GT
>> case 0x01: Imm = 0x03; break; // LE -> GE
>> @@ -3293,27 +3299,21 @@
>> default:
>> break;
>> }
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->getOperand(3).setImm(Imm);
>> - return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.getOperand(3).setImm(Imm);
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> case X86::VPERM2F128rr:
>> case X86::VPERM2I128rr: {
>> // Flip permute source immediate.
>> // Imm & 0x02: lo = if set, select Op1.lo/hi else Op0.lo/hi.
>> // Imm & 0x20: hi = if set, select Op1.lo/hi else Op0.lo/hi.
>> - unsigned Imm = MI->getOperand(3).getImm() & 0xFF;
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->getOperand(3).setImm(Imm ^ 0x22);
>> - return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> + unsigned Imm = MI.getOperand(3).getImm() & 0xFF;
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.getOperand(3).setImm(Imm ^ 0x22);
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> case X86::CMOVB16rr: case X86::CMOVB32rr: case X86::CMOVB64rr:
>> case X86::CMOVAE16rr: case X86::CMOVAE32rr: case X86::CMOVAE64rr:
>> @@ -3332,7 +3332,7 @@
>> case X86::CMOVO16rr: case X86::CMOVO32rr: case X86::CMOVO64rr:
>> case X86::CMOVNO16rr: case X86::CMOVNO32rr: case X86::CMOVNO64rr: {
>> unsigned Opc;
>> - switch (MI->getOpcode()) {
>> + switch (MI.getOpcode()) {
>> default: llvm_unreachable("Unreachable!");
>> case X86::CMOVB16rr: Opc = X86::CMOVAE16rr; break;
>> case X86::CMOVB32rr: Opc = X86::CMOVAE32rr; break;
>> @@ -3383,31 +3383,27 @@
>> case X86::CMOVNO32rr: Opc = X86::CMOVO32rr; break;
>> case X86::CMOVNO64rr: Opc = X86::CMOVO64rr; break;
>> }
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->setDesc(get(Opc));
>> - // Fallthrough intended.
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.setDesc(get(Opc));
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> default:
>> - if (isFMA3(MI->getOpcode())) {
>> + if (isFMA3(MI.getOpcode())) {
>> unsigned Opc = getFMA3OpcodeToCommuteOperands(MI, OpIdx1, OpIdx2);
>> if (Opc == 0)
>> return nullptr;
>> - if (NewMI) {
>> - MachineFunction &MF = *MI->getParent()->getParent();
>> - MI = MF.CloneMachineInstr(MI);
>> - NewMI = false;
>> - }
>> - MI->setDesc(get(Opc));
>> + auto &WorkingMI = cloneIfNew(MI);
>> + WorkingMI.setDesc(get(Opc));
>> + return TargetInstrInfo::commuteInstructionImpl(WorkingMI, /*NewMI=*/false,
>> + OpIdx1, OpIdx2);
>> }
>> +
>> return TargetInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
>> }
>> }
More information about the llvm-commits
mailing list