<div dir="ltr"><div><div>Hi Eli, <br><br>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.<br><br></div>Here's an example case that would change:<br><br>define double @fcmp_oeq_non_const(double %x, double %y, double %z1, double %z2) {<br>entry:<br> %z = fadd double %z1, %z2<br> %cmp = fcmp oeq double %y, %z<br> br i1 %cmp, label %if, label %return<br><br>if:<br> %z_again = fadd double %z1, %z2<br> %div = fdiv double %x, %z_again <--- don't replace %z_again with %y?<br> br label %return<br><br>return:<br> %retval = phi double [ %div, %if ], [ %x, %entry ]<br> ret double %retval<br><br><br></div>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.<br><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 15, 2015 at 5:41 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Thu, Jan 29, 2015 at 12:51 PM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br></span><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: spatel<br>
Date: Thu Jan 29 14:51:49 2015<br>
New Revision: 227491<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=227491&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=227491&view=rev</a><br>
Log:<br>
[GVN] don't propagate equality comparisons of FP zero (PR22376)<br>
<br>
In <a href="http://reviews.llvm.org/D6911" target="_blank">http://reviews.llvm.org/D6911</a>, we allowed GVN to propagate FP equalities<br>
to allow some simple value range optimizations. But that introduced a bug<br>
when comparing to -0.0 or 0.0: these compare equal even though they are not<br>
bitwise identical.<br>
<br>
This patch disallows propagating zero constants in equality comparisons.<br>
Fixes: <a href="http://llvm.org/bugs/show_bug.cgi?id=22376" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=22376</a><br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D7257" target="_blank">http://reviews.llvm.org/D7257</a><br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/GVN.cpp<br>
llvm/trunk/test/Transforms/GVN/edge.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=227491&r1=227490&r2=227491&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=227491&r1=227490&r2=227491&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Thu Jan 29 14:51:49 2015<br>
@@ -2182,9 +2182,16 @@ bool GVN::propagateEquality(Value *LHS,<br>
<br>
// Handle the floating point versions of equality comparisons too.<br>
if ((isKnownTrue && Cmp->getPredicate() == CmpInst::FCMP_OEQ) ||<br>
- (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE))<br>
- Worklist.push_back(std::make_pair(Op0, Op1));<br>
-<br>
+ (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE)) {<br>
+ // Floating point -0.0 and 0.0 compare equal, so we can't<br>
+ // propagate a constant based on that comparison.<br>
+ // FIXME: We should do this optimization if 'no signed zeros' is<br>
+ // applicable via an instruction-level fast-math-flag or some other<br>
+ // indicator that relaxed FP semantics are being used.<br>
+ if (!isa<ConstantFP>(Op1) || !cast<ConstantFP>(Op1)->isZero())<br>
+ Worklist.push_back(std::make_pair(Op0, Op1));<br>
+ }<br></blockquote><div><br></div></div></div><div>This test isn't right; it should be "if (isa<ConstantFP>(Op1) && !cast<ConstantFP>(Op1)->isZero())"<span class="HOEnZb"><font color="#888888"><br><br></font></span></div><span class="HOEnZb"><font color="#888888"><div>-Eli<br></div></font></span></div></div></div>
</blockquote></div><br></div>