[llvm-commits] [llvm] r156550 - in /llvm/trunk: lib/CodeGen/PeepholeOptimizer.cpp lib/Target/ARM/ARMBaseInstrInfo.cpp test/CodeGen/ARM/sub-cmp-peephole.ll
Manman Ren
mren at apple.com
Thu May 10 11:49:50 PDT 2012
Hi Nick,
I sent out the patch yesterday and thought it is safe to commit. Apparently, it is not.
Thanks for reviewing this.
I wil revert this patch and submit a new patch soon.
Manman
On May 10, 2012, at 11:01 AM, Nick Lewycky wrote:
> 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