[PATCH] [RFC] Lame fix for PR20376

[Chandler Carruth]

This is likely relevant to Tim's interests...


On Mon, Jul 21, 2014 at 6:55 PM, Peter Collingbourne <peter at pcc.me.uk>
wrote:

> 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.
>
> http://reviews.llvm.org/D4611
>
> Files:
>   lib/Target/X86/X86ISelLowering.cpp
>
> Index: lib/Target/X86/X86ISelLowering.cpp
> ===================================================================
> --- lib/Target/X86/X86ISelLowering.cpp
> +++ lib/Target/X86/X86ISelLowering.cpp
> @@ -19820,7 +19820,8 @@
>  //
>  // 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)))
> @@ -19887,12 +19888,21 @@
>      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));
> @@ -19965,7 +19975,7 @@
>
>    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))) {
> @@ -21803,7 +21813,7 @@
>
>    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);
> @@ -21825,7 +21835,7 @@
>
>    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,
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140721/a163ddb0/attachment.html>