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

[Sanjay Patel]

Hi Eli,

The change you are suggesting would prevent equality propagation of a
non-constant FP value. Is that the intention? If yes, why wouldn't we want
to do that optimization? We do that for the integer case.

Here's an example case that would change:

define double @fcmp_oeq_non_const(double %x, double %y, double %z1, double
%z2) {
entry:
 %z = fadd double %z1, %z2
 %cmp = fcmp oeq double %y, %z
 br i1 %cmp, label %if, label %return

if:
 %z_again = fadd double %z1, %z2
 %div = fdiv double %x, %z_again      <--- don't replace %z_again with %y?
 br label %return

return:
 %retval = phi double [ %div, %if ], [ %x, %entry ]
 ret double %retval


Note that another part of GVN will replace %z_again with %z even without
this patch. But I think replacing it with %y is always better because it
reduces the dependency chain for the fdiv.


On Sun, Feb 15, 2015 at 5:41 PM, Eli Friedman <eli.friedman at gmail.com>
wrote:

> 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/20150216/8d8776a3/attachment.html>