[llvm] r227491 - [GVN] don't propagate equality comparisons of FP zero (PR22376)

[Eli Friedman]

On Thu, Jan 29, 2015 at 12:51 PM, Sanjay Patel <spatel at rotateright.com>
wrote:

> Author: spatel
> Date: Thu Jan 29 14:51:49 2015
> New Revision: 227491
>
> URL: http://llvm.org/viewvc/llvm-project?rev=227491&view=rev
> Log:
> [GVN] don't propagate equality comparisons of FP zero (PR22376)
>
> In http://reviews.llvm.org/D6911, we allowed GVN to propagate FP
> equalities
> to allow some simple value range optimizations. But that introduced a bug
> when comparing to -0.0 or 0.0: these compare equal even though they are not
> bitwise identical.
>
> This patch disallows propagating zero constants in equality comparisons.
> Fixes: http://llvm.org/bugs/show_bug.cgi?id=22376
>
> Differential Revision: http://reviews.llvm.org/D7257
>
>
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/GVN.cpp
>     llvm/trunk/test/Transforms/GVN/edge.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=227491&r1=227490&r2=227491&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Thu Jan 29 14:51:49 2015
> @@ -2182,9 +2182,16 @@ bool GVN::propagateEquality(Value *LHS,
>
>        // Handle the floating point versions of equality comparisons too.
>        if ((isKnownTrue && Cmp->getPredicate() == CmpInst::FCMP_OEQ) ||
> -          (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE))
> -        Worklist.push_back(std::make_pair(Op0, Op1));
> -
> +          (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE)) {
> +        // Floating point -0.0 and 0.0 compare equal, so we can't
> +        // propagate a constant based on that comparison.
> +        // FIXME: We should do this optimization if 'no signed zeros' is
> +        // applicable via an instruction-level fast-math-flag or some
> other
> +        // indicator that relaxed FP semantics are being used.
> +        if (!isa<ConstantFP>(Op1) || !cast<ConstantFP>(Op1)->isZero())
> +          Worklist.push_back(std::make_pair(Op0, Op1));
> +      }
>

This test isn't right; it should be "if (isa<ConstantFP>(Op1) &&
!cast<ConstantFP>(Op1)->isZero())"

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150215/cc005996/attachment.html>