[PATCH] D21726: CodeGen: Use MachineInstr& in TargetInstrInfo, NFC
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 29 09:45:29 PDT 2016
"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);
> 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