<div dir="ltr">This is likely relevant to Tim's interests...</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 21, 2014 at 6:55 PM, Peter Collingbourne <span dir="ltr"><<a href="mailto:peter@pcc.me.uk" target="_blank">peter@pcc.me.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The problem (as I understand it) is the DAG eventually ends up representing<br>
a necessary EFLAGS copy. But we deliberately do not perform EFLAGS copies<br>
because they are impossible on x86 (see InstrEmitter.cpp:171 or thereabouts),<br>
so we end up using whatever happens to be in EFLAGS at the time of the branch.<br>
This problem does not end up affecting compares/overflow checks because<br>
they are schedule-independent, so we can easily move (or copy) the<br>
instruction to wherever is convenient.<br>
<br>
The workaround I came up with was to disable a simplification that may lead<br>
to such a copy occurring, specifically the one that eliminates SETCC nodes, if<br>
they refer to a CopyFromReg which may not be scheduled right before the branch,<br>
in the hope that the SETCC will be scheduled in the right place. This turned<br>
out to be not so great; a number of tests now fail because of extra copies.<br>
<br>
One other idea I had was a peephole MI pass that removes SETCC instructions<br>
if safe to do so. Does that sound like a good idea?<br>
<br>
There may be a much better way of solving this. I'm not a backend expert;<br>
this is what I have gleaned after staring at selection DAGs for far too<br>
long.<br>
<br>
<a href="http://reviews.llvm.org/D4611" target="_blank">http://reviews.llvm.org/D4611</a><br>
<br>
Files:<br>
lib/Target/X86/X86ISelLowering.cpp<br>
<br>
Index: lib/Target/X86/X86ISelLowering.cpp<br>
===================================================================<br>
--- lib/Target/X86/X86ISelLowering.cpp<br>
+++ lib/Target/X86/X86ISelLowering.cpp<br>
@@ -19820,7 +19820,8 @@<br>
//<br>
// where Op could be BRCOND or CMOV.<br>
//<br>
-static SDValue checkBoolTestSetCCCombine(SDValue Cmp, X86::CondCode &CC) {<br>
+static SDValue checkBoolTestSetCCCombine(SDValue Chain, SDValue Cmp,<br>
+ X86::CondCode &CC) {<br>
// Quit if not CMP and SUB with its value result used.<br>
if (Cmp.getOpcode() != X86ISD::CMP &&<br>
(Cmp.getOpcode() != X86ISD::SUB || Cmp.getNode()->hasAnyUseOfValue(0)))<br>
@@ -19887,12 +19888,21 @@<br>
assert(X86::CondCode(SetCC.getConstantOperandVal(0)) == X86::COND_B &&<br>
"Invalid use of SETCC_CARRY!");<br>
// FALL THROUGH<br>
- case X86ISD::SETCC:<br>
+ case X86ISD::SETCC: {<br>
+ // If the operand is CopyFromReg, applying this simplification may create<br>
+ // a situation later where we need to copy EFLAGS. Since this isn't<br>
+ // supported, we will generate incorrect code. To avoid this situation,<br>
+ // we forbid this simplification if the operand is CopyFromReg and it<br>
+ // is not directly chained to us.<br>
+ SDValue Op = SetCC.getOperand(1);<br>
+ if (Chain && Op.getOpcode() == ISD::CopyFromReg && Op.getValue(1) != Chain)<br>
+ break;<br>
// Set the condition code or opposite one if necessary.<br>
CC = X86::CondCode(SetCC.getConstantOperandVal(0));<br>
if (needOppositeCond)<br>
CC = X86::GetOppositeBranchCondition(CC);<br>
- return SetCC.getOperand(1);<br>
+ return Op;<br>
+ }<br>
case X86ISD::CMOV: {<br>
// Check whether false/true value has canonical one, i.e. 0 or 1.<br>
ConstantSDNode *FVal = dyn_cast<ConstantSDNode>(SetCC.getOperand(0));<br>
@@ -19965,7 +19975,7 @@<br>
<br>
SDValue Flags;<br>
<br>
- Flags = checkBoolTestSetCCCombine(Cond, CC);<br>
+ Flags = checkBoolTestSetCCCombine(SDValue(), Cond, CC);<br>
if (Flags.getNode() &&<br>
// Extra check as FCMOV only supports a subset of X86 cond.<br>
(FalseOp.getValueType() != MVT::f80 || hasFPCMov(CC))) {<br>
@@ -21803,7 +21813,7 @@<br>
<br>
SDValue Flags;<br>
<br>
- Flags = checkBoolTestSetCCCombine(EFLAGS, CC);<br>
+ Flags = checkBoolTestSetCCCombine(SDValue(), EFLAGS, CC);<br>
if (Flags.getNode()) {<br>
SDValue Cond = DAG.getConstant(CC, MVT::i8);<br>
return DAG.getNode(X86ISD::SETCC, DL, N->getVTList(), Cond, Flags);<br>
@@ -21825,7 +21835,7 @@<br>
<br>
SDValue Flags;<br>
<br>
- Flags = checkBoolTestSetCCCombine(EFLAGS, CC);<br>
+ Flags = checkBoolTestSetCCCombine(Chain, EFLAGS, CC);<br>
if (Flags.getNode()) {<br>
SDValue Cond = DAG.getConstant(CC, MVT::i8);<br>
return DAG.getNode(X86ISD::BRCOND, DL, N->getVTList(), Chain, Dest, Cond,<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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>
<br></blockquote></div><br></div>