[RFC] Lame fix for PR20376

[pcc]

Summary

The problem (as I understand it) is the DAG eventually ends up representing
a necessary EFLAGS copy. But we deliberately do not perform EFLAGS copies
because they are impossible on x86 (see InstrEmitter.cpp:171 or thereabouts),
so we end up using whatever happens to be in EFLAGS at the time of the branch.
This problem does not end up affecting compares/overflow checks because
they are schedule-independent, so we can easily move (or copy) the
instruction to wherever is convenient.

The workaround I came up with was to disable a simplification that may lead
to such a copy occurring, specifically the one that eliminates SETCC nodes, if
they refer to a CopyFromReg which may not be scheduled right before the branch,
in the hope that the SETCC will be scheduled in the right place. This turned
out to be not so great; a number of tests now fail because of extra copies.

One other idea I had was a peephole MI pass that removes SETCC instructions
if safe to do so. Does that sound like a good idea?

There may be a much better way of solving this. I'm not a backend expert;
this is what I have gleaned after staring at selection DAGs for far too
long.

[t.p.northover]

We actually do have a last-ditch EFLAGS copy available (though with worrying implications for the red-zone, if we ever start using it). It's used by test/CodeGen/Generic/2011-07-07-ScheduleDAGCrash.ll for example.

Enabling it more generally is probably just a matter of tweaking getCopyCost. Which might be an idea for robustness anyway.

But I'm not sure that actually affects the best solution here. I rather suspect performing the comparison again would be quicker in general. I suppose the question is how common and performance-critical such fragments are.

Cheers.

Tim.

[pcc]

• 0001-Change-the-cost-of-EFLAGS-copies.patch4 KBDownload

lib/Target/X86/X86ISelLowering.cpp

 // to (Op EFLAGS Cond)
 //
 // (Op (CMP (SETCC Cond EFLAGS) 0) EQ) or
 // (Op (CMP (SETCC Cond EFLAGS) 1) NEQ)
 // to (Op EFLAGS !Cond)
 //
 // where Op could be BRCOND or CMOV.
 //
-static SDValue checkBoolTestSetCCCombine(SDValue Cmp, X86::CondCode &CC) {
+static SDValue checkBoolTestSetCCCombine(SDValue Chain, SDValue Cmp,
+                                         X86::CondCode &CC) {
   // Quit if not CMP and SUB with its value result used.
   if (Cmp.getOpcode() != X86ISD::CMP &&
       (Cmp.getOpcode() != X86ISD::SUB || Cmp.getNode()->hasAnyUseOfValue(0)))
       return SDValue();
 
   // Quit if not used as a boolean value.
   if (CC != X86::COND_E && CC != X86::COND_NE)
     return SDValue();
case X86ISD::SETCC_CARRY:
     // simplify it if the result of SETCC_CARRY is not canonicalized to 0 or 1,
     // i.e. it's a comparison against true but the result of SETCC_CARRY is not
     // truncated to i1 using 'and'.
     if (checkAgainstTrue && !truncatedToBoolWithAnd)
       break;
     assert(X86::CondCode(SetCC.getConstantOperandVal(0)) == X86::COND_B &&
            "Invalid use of SETCC_CARRY!");
     // FALL THROUGH
-  case X86ISD::SETCC:
+  case X86ISD::SETCC: {
+    // If the operand is CopyFromReg, applying this simplification may create
+    // a situation later where we need to copy EFLAGS. Since this isn't
+    // supported, we will generate incorrect code. To avoid this situation,
+    // we forbid this simplification if the operand is CopyFromReg and it
+    // is not directly chained to us.
+    SDValue Op = SetCC.getOperand(1);
+    if (Chain && Op.getOpcode() == ISD::CopyFromReg && Op.getValue(1) != Chain)
+      break;
     // Set the condition code or opposite one if necessary.
     CC = X86::CondCode(SetCC.getConstantOperandVal(0));
     if (needOppositeCond)
       CC = X86::GetOppositeBranchCondition(CC);
-    return SetCC.getOperand(1);
+    return Op;
+  }
   case X86ISD::CMOV: {
     // Check whether false/true value has canonical one, i.e. 0 or 1.
     ConstantSDNode *FVal = dyn_cast<ConstantSDNode>(SetCC.getOperand(0));
     ConstantSDNode *TVal = dyn_cast<ConstantSDNode>(SetCC.getOperand(1));
     // Quit if true value is not a constant.
     if (!TVal)
       return SDValue();
     // Quit if false value is not a constant.
case X86ISD::BSF:
       // If operand of BSR / BSF are proven never zero, then ZF cannot be set.
       if (DAG.isKnownNeverZero(Cond.getOperand(0)))
         return (CC == X86::COND_E) ? FalseOp : TrueOp;
     }
   }
 
   SDValue Flags;
 
-  Flags = checkBoolTestSetCCCombine(Cond, CC);
+  Flags = checkBoolTestSetCCCombine(SDValue(), Cond, CC);
   if (Flags.getNode() &&
       // Extra check as FCMOV only supports a subset of X86 cond.
       (FalseOp.getValueType() != MVT::f80 || hasFPCMov(CC))) {
     SDValue Ops[] = { FalseOp, TrueOp,
                       DAG.getConstant(CC, MVT::i8), Flags };
     return DAG.getNode(X86ISD::CMOV, DL, N->getVTList(), Ops);
   }
 
static SDValue PerformSETCCCombine(SDNode *N, SelectionDAG &DAG,
   // Materialize "setb reg" as "sbb reg,reg", since it can be extended without
   // a zext and produces an all-ones bit which is more useful than 0/1 in some
   // cases.
   if (CC == X86::COND_B)
     return MaterializeSETB(DL, EFLAGS, DAG, N->getSimpleValueType(0));
 
   SDValue Flags;
 
-  Flags = checkBoolTestSetCCCombine(EFLAGS, CC);
+  Flags = checkBoolTestSetCCCombine(SDValue(), EFLAGS, CC);
   if (Flags.getNode()) {
     SDValue Cond = DAG.getConstant(CC, MVT::i8);
     return DAG.getNode(X86ISD::SETCC, DL, N->getVTList(), Cond, Flags);
   }
 
   return SDValue();
 }
 
 // Optimize branch condition evaluation.
 //
 static SDValue PerformBrCondCombine(SDNode *N, SelectionDAG &DAG,
                                     TargetLowering::DAGCombinerInfo &DCI,
                                     const X86Subtarget *Subtarget) {
   SDLoc DL(N);
   SDValue Chain = N->getOperand(0);
   SDValue Dest = N->getOperand(1);
   SDValue EFLAGS = N->getOperand(3);
   X86::CondCode CC = X86::CondCode(N->getConstantOperandVal(2));
 
   SDValue Flags;
 
-  Flags = checkBoolTestSetCCCombine(EFLAGS, CC);
+  Flags = checkBoolTestSetCCCombine(Chain, EFLAGS, CC);
   if (Flags.getNode()) {
     SDValue Cond = DAG.getConstant(CC, MVT::i8);
     return DAG.getNode(X86ISD::BRCOND, DL, N->getVTList(), Chain, Dest, Cond,
                        Flags);
   }
 
   return SDValue();
 }