[llvm-commits] [PATCH] fix PR13577, an issue caused by r161687

[Michael Liao]

On Sat, 2012-08-11 at 16:06 -0700, Chandler Carruth wrote:
> On Sat, Aug 11, 2012 at 3:49 PM, Michael Liao <michael.liao at intel.com>
> wrote:
>         Hi
>         
>         Sorry, PR13577 and recent failures of compiler-rt failures is
>         introduced
>         by r161687. The root cause is that FCMOV (only used for 80-bit
>         double)
>         only supports a subset of X86 conditions. The attached patch
>         fixes that
>         by checking whether it's a CMOV on f80 and whether the
>         condition is
>         valid. A minimal test case (copysign) for PR13577 is also
>         added.
> 
> 
> Hmm, my diagnosis of that PR may have been a bit off -- you should
> check whether this patch actually fixes it, as it seems to potentially
> predate r161687. Fixing it may however re-use this logic.
> 

I could compile lib/divxc3.c with llvm/clang with this patch. I also
checked out r161491 (no r161493 in llvm itself) and cannot reproduce
that issue. I could confirm that issue (PR13577) is caused by r161687
alone.

Yours
- Michael

> 
> Either way, that patch seems fine, and should at least fix the
> failures David sent you mail about. One nit-pick:
> 
> 
> @@ -13877,10 +13893,14 @@ static SDValue PerformCMOVCombine(SDNode *N,
> SelectionDAG &DAG,
>  
>    Flags = BoolTestSetCCCombine(Cond, CC);
>    if (Flags.getNode()) {
> -    SDValue Ops[] = { FalseOp, TrueOp,
> -                      DAG.getConstant(CC, MVT::i8), Flags };
> -    return DAG.getNode(X86ISD::CMOV, DL, N->getVTList(),
> -                       Ops, array_lengthof(Ops));
> +    // Check for FCMOV which only supports a subset of X86 cond.
> +    if (FalseOp.getValueType() != MVT::f80 ||
> +        IsValidFCMOVCondition(CC)) {
> 
> 
> Just combine this and the above condition:
> 
> 
> if (Flags.genNode() && (FalseOp.getValueType() != MVT::f80 || ...)) {
>   ...
> }
> 
> 
> +      SDValue Ops[] = { FalseOp, TrueOp,
> +                        DAG.getConstant(CC, MVT::i8), Flags };
> +      return DAG.getNode(X86ISD::CMOV, DL, N->getVTList(),
> +                         Ops, array_lengthof(Ops));
> +    }
>    }
> 
>