<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Sep 17, 2012, at 11:43 AM, Jan Voung <<a href="mailto:jvoung@google.com">jvoung@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Without the setIsDead(false), I get failures on:<div><br><div>* the CodeGen/X86/crash.ll test (unmodified)</div><div>* the CodeGen/X86/jump_sign.ll test (modified to have -verify-machineinstrs)</div><div><br></div><div><br>
</div><div><br></div><div><br></div><div>Here is the log from jump_sign.ll:<br><div><br></div><div>=======================<br></div><div><br></div><div><div>********************</div><div>FAIL: LLVM :: CodeGen/X86/jump_sign.ll (2769 of 6471)</div>
<div>******************** TEST 'LLVM :: CodeGen/X86/jump_sign.ll' FAILED ********************</div><div>Script:</div><div>--</div><div>/build/Release+Asserts/bin/llc < /src/test/CodeGen/X86/jump_sign.ll -march=x86 -mcpu=pentiumpro -verify-machineinstrs | /build/Release+Asserts/bin/FileCheck /src/llvm/test/CodeGen/X86/jump_sign.ll</div>
<div>--</div><div>Exit Code: 1</div><div>Command Output (stderr):</div><div>--</div><div><br></div><div># After codegen peephole optimization pass</div><div># Machine code for function h: SSA</div><div>Frame Objects:</div>
<div> fi#-2: size=4, align=4, fixed, at location [SP+8]</div><div> fi#-1: size=4, align=16, fixed, at location [SP+4]</div><div>Function Live Outs: %EAX</div><div><br></div><div>BB#0: derived from LLVM BB %entry</div><div>
<span class="" style="white-space:pre"> </span>%vreg0<def> = MOV32rm <fi#-2>, 1, %noreg, 0, %noreg; mem:LD4[FixedStack-2] GR32:%vreg0</div><div><span class="" style="white-space:pre"> </span>%vreg1<def> = MOV32rm <fi#-1>, 1, %noreg, 0, %noreg; mem:LD4[FixedStack-1](align=16) GR32:%vreg1</div>
<div><span class="" style="white-space:pre"> </span>%vreg3<def> = MOV32r0 %EFLAGS<imp-def,dead>; GR32:%vreg3</div><div><span class="" style="white-space:pre"> </span>%vreg2<def,tied1> = SUB32rr %vreg1<tied0>, %vreg0, %EFLAGS<imp-def,dead>; GR32:%vreg2,%vreg1,%vreg0</div>
<div><span class="" style="white-space:pre"> </span>%vreg5<def,tied1> = CMOVG32rr %vreg3<tied0>, %vreg2<kill>, %EFLAGS<imp-use>; GR32:%vreg5,%vreg3,%vreg2</div><div><span class="" style="white-space:pre"> </span>%EAX<def> = COPY %vreg5; GR32:%vreg5</div>
<div><span class="" style="white-space:pre"> </span>RET</div><div><br></div><div># End machine code for function h.</div><div><br></div><div>*** Bad machine code: Using an undefined physical register ***</div><div>- function: h</div>
<div>- basic block: BB#0 entry (0x3194ab0)</div><div>- instruction: %vreg5<def,tied1> = CMOVG32rr %vreg3<tied0>, %vreg2<kill>, %EFLAGS<imp-use>; GR32:%vreg5,%vreg3,%vreg2</div><div>- operand 3: %EFLAGS<imp-use></div>
<div>LLVM ERROR: Found 1 machine code errors.</div><div>FileCheck error: '-' is empty.</div><div><br></div><div><br></div><div>=======================</div><div><br></div><div>"-debug" shows this bit of text before the error:</div>
<div><br></div><div><div># Machine code for function h: SSA</div><div>Frame Objects:</div><div> fi#-2: size=4, align=4, fixed, at location [SP+8]</div><div> fi#-1: size=4, align=16, fixed, at location [SP+4]</div><div>Function Live Outs: %EAX</div>
<div><br></div><div>0B BB#0: derived from LLVM BB %entry</div><div>16B %vreg0<def> = MOV32rm <fi#-2>, 1, %noreg, 0, %noreg; mem:LD4[FixedStack-2] GR32:%vreg0</div><div>32B %vreg1<def> = MOV32rm <fi#-1>, 1, %noreg, 0, %noreg; mem:LD4[FixedStack-1](align=16) GR32:%vreg1</div>
<div>48B %vreg2<def,tied1> = SUB32rr %vreg1<tied0>, %vreg0, %EFLAGS<imp-def,dead>; GR32:%vreg2,%vreg1,%vreg0</div><div>64B %vreg3<def> = MOV32r0 %EFLAGS<imp-def,dead>; GR32:%vreg3</div>
<div>80B %vreg4<def,tied1> = SUB32rr %vreg0<tied0>, %vreg1, %EFLAGS<imp-def>; GR32:%vreg4,%vreg0,%vreg1</div><div>96B %vreg5<def,tied1> = CMOVL32rr %vreg3<tied0>, %vreg2<kill>, %EFLAGS<imp-use>; GR32:%vreg5,%vreg3,%vreg2</div>
<div>112B %EAX<def> = COPY %vreg5; GR32:%vreg5</div><div>128B RET</div><div><br></div><div># End machine code for function h.</div></div><div><br></div><div>=======================</div>
<div><br></div><div><br></div><div>So before the pass runs, there was another SUB32rr which redefined EFLAGS and that is the def that is live for the CMOV. I think once you remove that redundant definition in favor of the earlier one, you have to make the earlier one live?</div></div></div></div></blockquote>You are right. Looks like setting IsDead to false is necessary.</div><div>Your patch looks good to me.</div><div><br></div><div>Thanks</div><div>Manman<br><blockquote type="cite"><div><div><div>
<div><br></div><div>- Jan</div><div><br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 17, 2012 at 11:21 AM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div>Hi Jan,<div>
<br></div><div>The patch looks good except it is weird that you have to set IsDead to false right after setting IsDef to true.</div><div>Could you look deeper why that is needed? Can you dump the related info?</div><div><br>
</div><div>Thanks,</div><div>Manman</div><div><br><div><div><div class="h5"><div>On Sep 16, 2012, at 4:06 PM, Jan Voung <<a href="mailto:jvoung@google.com" target="_blank">jvoung@google.com</a>> wrote:</div><br></div>
</div><blockquote type="cite"><div><div class="h5">Thanks, here's a patch that adds that.<div><br></div><div>One of the "make check" tests that used "-verify-machineinstrs" failed, showing that the def of EFLAGS from a DEC was dead before the new use. I ended up adding a setIsDead(false) right after the setIsDef(true). Does that seem alright?</div>
<div><br></div><div>-Jan</div><div><br><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 14, 2012 at 12:55 PM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>
On Sep 14, 2012, at 11:27 AM, Jan Voung <<a href="mailto:jvoung@google.com" target="_blank">jvoung@google.com</a>> wrote:</div>
<br><blockquote type="cite"><br><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 18, 2012 at 2:40 PM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Author: mren<br>
Date: Wed Jul 18 16:40:01 2012<br>
New Revision: 160454<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=160454&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=160454&view=rev</a><br>
Log:<br>
X86: remove redundant cmp against zero.<br>
<br>
Updated OptimizeCompare in peephole to remove redundant cmp against zero.<br>
We only remove Compare if CF and OF are not used.<br>
<br>
<a>rdar://11855129</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Target/X86/X86InstrArithmetic.td<br>
llvm/trunk/lib/Target/X86/X86InstrInfo.cpp<br>
llvm/trunk/test/CodeGen/X86/jump_sign.ll<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86InstrArithmetic.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=160454&r1=160453&r2=160454&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=160454&r1=160453&r2=160454&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86InstrArithmetic.td (original)<br>
+++ llvm/trunk/lib/Target/X86/X86InstrArithmetic.td Wed Jul 18 16:40:01 2012<br>
@@ -1156,7 +1156,7 @@<br>
def X86testpat : PatFrag<(ops node:$lhs, node:$rhs),<br>
(X86cmp (and_su node:$lhs, node:$rhs), 0)>;<br>
<br>
-let Defs = [EFLAGS] in {<br>
+let isCompare = 1, Defs = [EFLAGS] in {<br>
let isCommutable = 1 in {<br>
def TEST8rr : BinOpRR_F<0x84, "test", Xi8 , X86testpat, MRMSrcReg>;<br>
def TEST16rr : BinOpRR_F<0x84, "test", Xi16, X86testpat, MRMSrcReg>;<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrInfo.cpp?rev=160454&r1=160453&r2=160454&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrInfo.cpp?rev=160454&r1=160453&r2=160454&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86InstrInfo.cpp (original)<br>
+++ llvm/trunk/lib/Target/X86/X86InstrInfo.cpp Wed Jul 18 16:40:01 2012<br>
@@ -3046,6 +3046,17 @@<br>
CmpMask = ~0;<br>
CmpValue = 0;<br>
return true;<br>
+ case X86::TEST8rr:<br>
+ case X86::TEST16rr:<br>
+ case X86::TEST32rr:<br>
+ case X86::TEST64rr:<br>
+ SrcReg = MI->getOperand(0).getReg();<br>
+ if (MI->getOperand(1).getReg() != SrcReg) return false;<br>
+ // Compare against zero.<br>
+ SrcReg2 = 0;<br>
+ CmpMask = ~0;<br>
+ CmpValue = 0;<br>
+ return true;<br>
}<br>
return false;<br>
}<br>
@@ -3093,6 +3104,40 @@<br>
return false;<br>
}<br>
<br>
+/// isDefConvertible - check whether the definition can be converted<br>
+/// to remove a comparison against zero.<br>
+inline static bool isDefConvertible(MachineInstr *MI) {<br>
+ switch (MI->getOpcode()) {<br>
+ default: return false;<br>
+ case X86::SUB64ri32: case X86::SUB64ri8: case X86::SUB32ri:<br>
+ case X86::SUB32ri8: case X86::SUB16ri: case X86::SUB16ri8:<br>
+ case X86::SUB8ri: case X86::SUB64rr: case X86::SUB32rr:<br>
+ case X86::SUB16rr: case X86::SUB8rr: case X86::SUB64rm:<br>
+ case X86::SUB32rm: case X86::SUB16rm: case X86::SUB8rm:<br>
+ case X86::ADD64ri32: case X86::ADD64ri8: case X86::ADD32ri:<br>
+ case X86::ADD32ri8: case X86::ADD16ri: case X86::ADD16ri8:<br>
+ case X86::ADD8ri: case X86::ADD64rr: case X86::ADD32rr:<br>
+ case X86::ADD16rr: case X86::ADD8rr: case X86::ADD64rm:<br>
+ case X86::ADD32rm: case X86::ADD16rm: case X86::ADD8rm:<br>
+ case X86::AND64ri32: case X86::AND64ri8: case X86::AND32ri:<br>
+ case X86::AND32ri8: case X86::AND16ri: case X86::AND16ri8:<br>
+ case X86::AND8ri: case X86::AND64rr: case X86::AND32rr:<br>
+ case X86::AND16rr: case X86::AND8rr: case X86::AND64rm:<br>
+ case X86::AND32rm: case X86::AND16rm: case X86::AND8rm:<br>
+ case X86::XOR64ri32: case X86::XOR64ri8: case X86::XOR32ri:<br>
+ case X86::XOR32ri8: case X86::XOR16ri: case X86::XOR16ri8:<br>
+ case X86::XOR8ri: case X86::XOR64rr: case X86::XOR32rr:<br>
+ case X86::XOR16rr: case X86::XOR8rr: case X86::XOR64rm:<br>
+ case X86::XOR32rm: case X86::XOR16rm: case X86::XOR8rm:<br>
+ case X86::OR64ri32: case X86::OR64ri8: case X86::OR32ri:<br>
+ case X86::OR32ri8: case X86::OR16ri: case X86::OR16ri8:<br>
+ case X86::OR8ri: case X86::OR64rr: case X86::OR32rr:<br>
+ case X86::OR16rr: case X86::OR8rr: case X86::OR64rm:<br>
+ case X86::OR32rm: case X86::OR16rm: case X86::OR8rm:<br></blockquote><div><br></div><div>Hi Manman,</div><div><br></div><div>Does it make sense to add the DEC/INC variants to isDefConvertible too?</div><div>Or, is that not safe? I just noticed that if tests weren't optimized earlier</div>
<div>(e.g., in X86ISelLowering::EmitTest()), then you could often end up with</div><div>DEC/INCs at this point.</div></div></div></blockquote></div>I think it makes sense and is safe to add "INC/DEC" here.</div>
<div><br></div><div>Thanks,</div><div>Manman</div><div><div><br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>- Jan</div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto; ">
+ return true;<br>
+ }<br>
+}<br>
+<br>
/// optimizeCompareInstr - Check if there exists an earlier instruction that<br>
/// operates on the same source operands and sets flags in the same way as<br>
/// Compare; remove Compare if possible.<br>
@@ -3107,6 +3152,13 @@<br>
// CmpInstr is the first instruction of the BB.<br>
MachineBasicBlock::iterator I = CmpInstr, Def = MI;<br>
<br>
+ // If we are comparing against zero, check whether we can use MI to update<br>
+ // EFLAGS. If MI is not in the same BB as CmpInstr, do not optimize.<br>
+ bool IsCmpZero = (SrcReg2 == 0 && CmpValue == 0);<br>
+ if (IsCmpZero && (MI->getParent() != CmpInstr->getParent() ||<br>
+ !isDefConvertible(MI)))<br>
+ return false;<br>
+<br>
// We are searching for an earlier instruction that can make CmpInstr<br>
// redundant and that instruction will be saved in Sub.<br>
MachineInstr *Sub = NULL;<br>
@@ -3126,7 +3178,8 @@<br>
for (; RI != RE; ++RI) {<br>
MachineInstr *Instr = &*RI;<br>
// Check whether CmpInstr can be made redundant by the current instruction.<br>
- if (isRedundantFlagInstr(CmpInstr, SrcReg, SrcReg2, CmpValue, Instr)) {<br>
+ if (!IsCmpZero &&<br>
+ isRedundantFlagInstr(CmpInstr, SrcReg, SrcReg2, CmpValue, Instr)) {<br>
Sub = Instr;<br>
break;<br>
}<br>
@@ -3153,7 +3206,7 @@<br>
}<br>
<br>
// Return false if no candidates exist.<br>
- if (!Sub)<br>
+ if (!IsCmpZero && !Sub)<br>
return false;<br>
<br>
bool IsSwapped = (SrcReg2 != 0 && Sub->getOperand(1).getReg() == SrcReg2 &&<br>
@@ -3177,13 +3230,10 @@<br>
continue;<br>
<br>
// EFLAGS is used by this instruction.<br>
- if (IsSwapped) {<br>
- // If we have SUB(r1, r2) and CMP(r2, r1), the condition code needs<br>
- // to be changed from r2 > r1 to r1 < r2, from r2 < r1 to r1 > r2, etc.<br>
- // We decode the condition code from opcode, swap the condition code,<br>
- // and synthesize the new opcode.<br>
- bool OpcIsSET = false;<br>
- X86::CondCode OldCC;<br>
+ X86::CondCode OldCC;<br>
+ bool OpcIsSET = false;<br>
+ if (IsCmpZero || IsSwapped) {<br>
+ // We decode the condition code from opcode.<br>
if (Instr.isBranch())<br>
OldCC = getCondFromBranchOpc(Instr.getOpcode());<br>
else {<br>
@@ -3194,6 +3244,22 @@<br>
OldCC = getCondFromCMovOpc(Instr.getOpcode());<br>
}<br>
if (OldCC == X86::COND_INVALID) return false;<br>
+ }<br>
+ if (IsCmpZero) {<br>
+ switch (OldCC) {<br>
+ default: break;<br>
+ case X86::COND_A: case X86::COND_AE:<br>
+ case X86::COND_B: case X86::COND_BE:<br>
+ case X86::COND_G: case X86::COND_GE:<br>
+ case X86::COND_L: case X86::COND_LE:<br>
+ case X86::COND_O: case X86::COND_NO:<br>
+ // CF and OF are used, we can't perform this optimization.<br>
+ return false;<br>
+ }<br>
+ } else if (IsSwapped) {<br>
+ // If we have SUB(r1, r2) and CMP(r2, r1), the condition code needs<br>
+ // to be changed from r2 > r1 to r1 < r2, from r2 < r1 to r1 > r2, etc.<br>
+ // We swap the condition code and synthesize the new opcode.<br>
X86::CondCode NewCC = getSwappedCondition(OldCC);<br>
if (NewCC == X86::COND_INVALID) return false;<br>
<br>
@@ -3223,7 +3289,7 @@<br>
<br>
// If EFLAGS is not killed nor re-defined, we should check whether it is<br>
// live-out. If it is live-out, do not optimize.<br>
- if (IsSwapped && !IsSafe) {<br>
+ if ((IsCmpZero || IsSwapped) && !IsSafe) {<br>
MachineBasicBlock *MBB = CmpInstr->getParent();<br>
for (MachineBasicBlock::succ_iterator SI = MBB->succ_begin(),<br>
SE = MBB->succ_end(); SI != SE; ++SI)<br>
@@ -3231,6 +3297,8 @@<br>
return false;<br>
}<br>
<br>
+ // The instruction to be updated is either Sub or MI.<br>
+ Sub = IsCmpZero ? MI : Sub;<br>
// Move Movr0Inst to the place right before Sub.<br>
if (Movr0Inst) {<br>
Sub->getParent()->remove(Movr0Inst);<br>
@@ -3238,10 +3306,11 @@<br>
}<br>
<br>
// Make sure Sub instruction defines EFLAGS.<br>
- assert(Sub->getNumOperands() >= 4 && Sub->getOperand(3).isReg() &&<br>
- Sub->getOperand(3).getReg() == X86::EFLAGS &&<br>
- "EFLAGS should be the 4th operand of SUBrr or SUBri.");<br>
- Sub->getOperand(3).setIsDef(true);<br>
+ assert(Sub->getNumOperands() >= 2 &&<br>
+ Sub->getOperand(Sub->getNumOperands()-1).isReg() &&<br>
+ Sub->getOperand(Sub->getNumOperands()-1).getReg() == X86::EFLAGS &&<br>
+ "EFLAGS should be the last operand of SUB, ADD, OR, XOR, AND");<br>
+ Sub->getOperand(Sub->getNumOperands()-1).setIsDef(true);<br>
CmpInstr->eraseFromParent();<br>
<br>
// Modify the condition code of instructions in OpsToUpdate.<br>
<br>
Modified: llvm/trunk/test/CodeGen/X86/jump_sign.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/jump_sign.ll?rev=160454&r1=160453&r2=160454&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/jump_sign.ll?rev=160454&r1=160453&r2=160454&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/CodeGen/X86/jump_sign.ll (original)<br>
+++ llvm/trunk/test/CodeGen/X86/jump_sign.ll Wed Jul 18 16:40:01 2012<br>
@@ -202,3 +202,14 @@<br>
if.else.i104: ; preds = %if.then44<br>
ret void<br>
}<br>
+; <a>rdar://11855129</a><br>
+define i32 @p(i32 %a, i32 %b) nounwind {<br>
+entry:<br>
+; CHECK: p:<br>
+; CHECK-NOT: test<br>
+; CHECK: cmovs<br>
+ %add = add nsw i32 %b, %a<br>
+ %cmp = icmp sgt i32 %add, 0<br>
+ %add. = select i1 %cmp, i32 %add, i32 0<br>
+ ret i32 %add.<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br></div></div>
</blockquote></div><br></div></div></div>
</div></div><span><cmp_dec_inc.patch></span></blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></body></html>