[llvm-commits] [llvm] r156550 - in /llvm/trunk: lib/CodeGen/PeepholeOptimizer.cpp lib/Target/ARM/ARMBaseInstrInfo.cpp test/CodeGen/ARM/sub-cmp-peephole.ll
Nick Lewycky
nicholas at mxc.ca
Thu May 10 11:01:04 PDT 2012
Manman Ren wrote:
> Author: mren
> Date: Thu May 10 11:48:21 2012
> New Revision: 156550
>
> URL: http://llvm.org/viewvc/llvm-project?rev=156550&view=rev
> Log:
> ARM: peephole optimization to remove cmp instruction
>
> This patch will optimize the following cases:
> sub r1, r3 | sub r1, imm
> cmp r3, r1 or cmp r1, r3 | cmp r1, imm
> bge L1
>
> TO
> subs r1, r3
> bge L1 or ble L1
>
> If the branch instruction can use flag from "sub", then we can replace
> "sub" with "subs" and eliminate the "cmp" instruction.
>
> rdar: 10734411
>
> Added:
> llvm/trunk/test/CodeGen/ARM/sub-cmp-peephole.ll
> Modified:
> llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp
> llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp
>
> Modified: llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp?rev=156550&r1=156549&r2=156550&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp (original)
> +++ llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp Thu May 10 11:48:21 2012
> @@ -31,6 +31,15 @@
> // same flag that the "cmp" instruction sets and that "bz" uses, then we can
> // eliminate the "cmp" instruction.
> //
> +// Another instance, in this code:
> +//
> +// sub r1, r3 | sub r1, imm
> +// cmp r3, r1 or cmp r1, r3 | cmp r1, imm
> +// bge L1
> +//
> +// If the branch instruction can use flag from "sub", then we can replace
> +// "sub" with "subs" and eliminate the "cmp" instruction.
> +//
> // - Optimize Bitcast pairs:
> //
> // v1 = bitcast v0
>
> Modified: llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp?rev=156550&r1=156549&r2=156550&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp Thu May 10 11:48:21 2012
> @@ -1753,6 +1753,12 @@
> CmpMask = ~0;
> CmpValue = MI->getOperand(1).getImm();
> return true;
> + case ARM::CMPrr:
> + case ARM::t2CMPrr:
> + SrcReg = MI->getOperand(0).getReg();
> + CmpMask = ~0;
> + CmpValue = 0;
> + return true;
> case ARM::TSTri:
> case ARM::t2TSTri:
> SrcReg = MI->getOperand(0).getReg();
> @@ -1794,12 +1800,13 @@
> }
>
> /// OptimizeCompareInstr - Convert the instruction supplying the argument to the
> -/// comparison into one that sets the zero bit in the flags register.
> +/// comparison into one that sets the zero bit in the flags register. Convert
> +/// the SUBrr(r1,r2)|Subri(r1,CmpValue) instruction into one that sets the flags
> +/// register and remove the CMPrr(r1,r2)|CMPrr(r2,r1)|CMPri(r1,CmpValue)
> +/// instruction.
> bool ARMBaseInstrInfo::
> OptimizeCompareInstr(MachineInstr *CmpInstr, unsigned SrcReg, int CmpMask,
> int CmpValue, const MachineRegisterInfo *MRI) const {
> - if (CmpValue != 0)
> - return false;
>
> MachineRegisterInfo::def_iterator DI = MRI->def_begin(SrcReg);
> if (llvm::next(DI) != MRI->def_end())
> @@ -1825,18 +1832,36 @@
> }
> }
>
> - // Conservatively refuse to convert an instruction which isn't in the same BB
> - // as the comparison.
> - if (MI->getParent() != CmpInstr->getParent())
> - return false;
> -
> - // Check that CPSR isn't set between the comparison instruction and the one we
> - // want to change.
> - MachineBasicBlock::iterator I = CmpInstr,E = MI, B = MI->getParent()->begin();
> + // Get ready to iterate backward from CmpInstr.
> + MachineBasicBlock::iterator I = CmpInstr,E = MI,
Please add a space after the comma, it wasn't there previously just to
pack things to fit on 80 columns.
> + B = CmpInstr->getParent()->begin();
>
> // Early exit if CmpInstr is at the beginning of the BB.
> if (I == B) return false;
>
> + // There are two possible candidates which can be changed to set CPSR:
> + // One is MI, the other is a SUB instruction.
> + // For CMPrr(r1,r2), we are looking for SUB(r1,r2) or SUB(r2,r1).
> + // For CMPri(r1, CmpValue), we are looking for SUBri(r1, CmpValue).
> + MachineInstr *Sub = NULL;
> + unsigned SrcReg2 = 0;
> + if (CmpInstr->getOpcode() == ARM::CMPrr ||
> + CmpInstr->getOpcode() == ARM::t2CMPrr) {
> + SrcReg2 = CmpInstr->getOperand(1).getReg();
> + // MI is not a candidate for CMPrr.
> + MI = NULL;
> + } else if (MI->getParent() != CmpInstr->getParent() || CmpValue != 0)
> + // Conservatively refuse to convert an instruction which isn't in the same
> + // BB as the comparison.
> + // For CMPri, we need to check Sub, thus we can't return here.
> + if(CmpInstr->getOpcode() == ARM::CMPri ||
> + CmpInstr->getOpcode() == ARM::t2CMPri)
> + MI = NULL;
> + else
> + return false;
GCC is warning that the 'else' here is ambiguous, please add braces
around the multi-line else-if. I'd do it myself, but I suspect that the
logic is backwards as written, and ought to be:
if (CmpInstr->getOpcode() == ARM::CMPrr ||
CmpInstr->getOpcode() == ARM::t2CMPrr) {
SrcReg2 = CmpInstr->getOperand(1).getReg();
// MI is not a candidate for CMPrr.
MI = NULL;
} else if ((MI->getParent() != CmpInstr->getParent() || CmpValue != 0) &&
(CmpInstr->getOpcode() == ARM::CMPri ||
CmpInstr->getOpcode() == ARM::t2CMPri))
// Conservatively refuse to convert an instruction which isn't in
the same
// BB as the comparison.
// For CMPri, we need to check Sub, thus we can't return here.
MI = NULL;
} else
return false;
? I'll let you handle it.
> +
> + // Check that CPSR isn't set between the comparison instruction and the one we
> + // want to change. At the same time, search for Sub.
> --I;
> for (; I != E; --I) {
> const MachineInstr&Instr = *I;
> @@ -1853,12 +1878,38 @@
> return false;
> }
>
> + // Check whether the current instruction is SUB(r1,r2) or SUB(r2,r1).
> + if (SrcReg2 != 0&& Instr.getOpcode() == ARM::SUBrr&&
> + ((Instr.getOperand(1).getReg() == SrcReg&&
> + Instr.getOperand(2).getReg() == SrcReg2) ||
> + (Instr.getOperand(1).getReg() == SrcReg2&&
> + Instr.getOperand(2).getReg() == SrcReg))) {
> + Sub =&*I;
> + break;
> + }
> +
> + // Check whether the current instruction is SUBri(r1,CmpValue).
Space after comma
> + if ((CmpInstr->getOpcode() == ARM::CMPri ||
> + CmpInstr->getOpcode() == ARM::t2CMPri)&&
> + Instr.getOpcode() == ARM::SUBri&& CmpValue != 0&&
> + Instr.getOperand(1).getReg() == SrcReg&&
> + Instr.getOperand(2).getImm() == CmpValue) {
> + Sub =&*I;
> + break;
> + }
> +
> if (I == B)
> // The 'and' is below the comparison instruction.
> return false;
> }
>
> - // Set the "zero" bit in CPSR.
> + // Return false if no candidates exist.
> + if (!MI&& !Sub)
> + return false;
> +
> + // The single candidate is called MI.
> + if (!MI) MI = Sub;
> +
> switch (MI->getOpcode()) {
> default: break;
> case ARM::RSBrr:
> @@ -1894,13 +1945,16 @@
> case ARM::EORri:
> case ARM::t2EORrr:
> case ARM::t2EORri: {
> - // Scan forward for the use of CPSR, if it's a conditional code requires
> + // Scan forward for the use of CPSR
> + // When checking against MI: if it's a conditional code requires
> // checking of V bit, then this is not safe to do. If we can't find the
> // CPSR use (i.e. used in another block), then it's not safe to perform
> // the optimization.
> + // When checking against Sub, we handle the condition codes GE, LT, GT, LE.
> + SmallVector<MachineOperand*, 4> OpndsToUpdate;
I'd prefer OpsToUpdate or OperandsToUpdate. I haven't seen "Opnds" in
the codebase yet. :)
Nick
> bool isSafe = false;
> I = CmpInstr;
> - E = MI->getParent()->end();
> + E = CmpInstr->getParent()->end();
> while (!isSafe&& ++I != E) {
> const MachineInstr&Instr = *I;
> for (unsigned IO = 0, EO = Instr.getNumOperands();
> @@ -1918,28 +1972,65 @@
> }
> // Condition code is after the operand before CPSR.
> ARMCC::CondCodes CC = (ARMCC::CondCodes)Instr.getOperand(IO-1).getImm();
> - switch (CC) {
> - default:
> - isSafe = true;
> - break;
> - case ARMCC::VS:
> - case ARMCC::VC:
> - case ARMCC::GE:
> - case ARMCC::LT:
> - case ARMCC::GT:
> - case ARMCC::LE:
> - return false;
> - }
> + if (Sub)
> + switch (CC) {
> + default:
> + return false;
> + case ARMCC::GE:
> + case ARMCC::LT:
> + case ARMCC::GT:
> + case ARMCC::LE:
> + // If we have SUB(r1, r2) and CMP(r2, r1), the condition code based
> + // on CMP needs to be updated to be based on SUB.
> + // Push the condition code operands to OpndsToUpdate.
> + // If it is safe to remove CmpInstr, the condition code of these
> + // operands will be modified.
> + if (SrcReg2 != 0&& Sub->getOperand(1).getReg() == SrcReg2&&
> + Sub->getOperand(2).getReg() == SrcReg)
> + OpndsToUpdate.push_back(&((*I).getOperand(IO-1)));
> + break;
> + }
> + else
> + switch (CC) {
> + default:
> + isSafe = true;
> + break;
> + case ARMCC::VS:
> + case ARMCC::VC:
> + case ARMCC::GE:
> + case ARMCC::LT:
> + case ARMCC::GT:
> + case ARMCC::LE:
> + return false;
> + }
> }
> }
>
> - if (!isSafe)
> + // If the candidate is Sub, we may exit the loop at end of the basic block.
> + // In that case, it is still safe to remove CmpInstr.
> + if (!isSafe&& !Sub)
> return false;
>
> // Toggle the optional operand to CPSR.
> MI->getOperand(5).setReg(ARM::CPSR);
> MI->getOperand(5).setIsDef(true);
> CmpInstr->eraseFromParent();
> +
> + // Modify the condition code of operands in OpndsToUpdate.
> + // Since we have SUB(r1, r2) and CMP(r2, r1), the condition code needs to
> + // be changed from r2> r1 to r1< r2, from r2< r1 to r1> r2, etc.
> + for (unsigned i = 0; i< OpndsToUpdate.size(); i++) {
> + ARMCC::CondCodes CC = (ARMCC::CondCodes)OpndsToUpdate[i]->getImm();
> + ARMCC::CondCodes NewCC;
> + switch(CC) {
> + default: break;
> + case ARMCC::GE: NewCC = ARMCC::LE; break;
> + case ARMCC::LT: NewCC = ARMCC::GT; break;
> + case ARMCC::GT: NewCC = ARMCC::LT; break;
> + case ARMCC::LE: NewCC = ARMCC::GT; break;
> + }
> + OpndsToUpdate[i]->setImm(NewCC);
> + }
> return true;
> }
> }
>
> Added: llvm/trunk/test/CodeGen/ARM/sub-cmp-peephole.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/sub-cmp-peephole.ll?rev=156550&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/sub-cmp-peephole.ll (added)
> +++ llvm/trunk/test/CodeGen/ARM/sub-cmp-peephole.ll Thu May 10 11:48:21 2012
> @@ -0,0 +1,34 @@
> +; RUN: llc< %s -march=arm | FileCheck %s
> +
> +define i32 @f(i32 %a, i32 %b) nounwind ssp {
> +entry:
> +; CHECK: _f:
> +; CHECK: subs
> +; CHECK-NOT: cmp
> + %cmp = icmp sgt i32 %a, %b
> + %sub = sub nsw i32 %a, %b
> + %sub. = select i1 %cmp, i32 %sub, i32 0
> + ret i32 %sub.
> +}
> +
> +define i32 @g(i32 %a, i32 %b) nounwind ssp {
> +entry:
> +; CHECK: _g:
> +; CHECK: subs
> +; CHECK-NOT: cmp
> + %cmp = icmp slt i32 %a, %b
> + %sub = sub nsw i32 %b, %a
> + %sub. = select i1 %cmp, i32 %sub, i32 0
> + ret i32 %sub.
> +}
> +
> +define i32 @h(i32 %a, i32 %b) nounwind ssp {
> +entry:
> +; CHECK: _h:
> +; CHECK: subs
> +; CHECK-NOT: cmp
> + %cmp = icmp sgt i32 %a, 3
> + %sub = sub nsw i32 %a, 3
> + %sub. = select i1 %cmp, i32 %sub, i32 %b
> + ret i32 %sub.
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list