<div dir="ltr">It's a correctness issue.<div><br></div><div>In general one cannot know whether or not a divide instruction is safe to execute because <span style="font-family:arial,sans-serif;font-size:13px">isKnownNonZero</span> returns true for poison.</div><div><br></div><div>It is possible for the right hand side of the divide to always be poison in a valid program so long as the divide cannot ever be executed.</div><div><br></div><div>For instructions which trap, we need a stronger predicate than 'isKnownNonZero': we need a hypothetical 'isKnownNeverToBeUndef'.</div><div><br></div><div>I doubt a correct implementation of isKnownNeverToBeUndef will ever fire because it must be incredibly conservative: function arguments might silently cary poison in them making them unsafe.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 5, 2014 at 5:05 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">David,<br>
<br>
Is this a correctness issue or a heuristic? The description doesn't say.<br>
<br>
I find this change potentially concerning since it will heavily influence LICM behaviour.<span class="HOEnZb"><font color="#888888"><br>
<br>
Philip</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 11/04/2014 03:49 PM, David Majnemer wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: majnemer<br>
Date: Tue Nov 4 17:49:08 2014<br>
New Revision: 221318<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=221318&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=221318&view=rev</a><br>
Log:<br>
Analysis: Make isSafeToSpeculativelyExecute fire less for divides<br>
<br>
Divides and remainder operations do not behave like other operations<br>
when they are given poison: they turn into undefined behavior.<br>
<br>
It's really hard to know if the operands going into a div are or are not<br>
poison. Because of this, we should only choose to speculate if there<br>
are constant operands which we can easily reason about.<br>
<br>
This fixes PR21412.<br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/<u></u>ValueTracking.cpp<br>
llvm/trunk/test/Transforms/<u></u>LICM/speculate.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/<u></u>ValueTracking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=221318&r1=221317&r2=221318&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>Analysis/ValueTracking.cpp?<u></u>rev=221318&r1=221317&r2=<u></u>221318&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Analysis/<u></u>ValueTracking.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/<u></u>ValueTracking.cpp Tue Nov 4 17:49:08 2014<br>
@@ -2549,23 +2549,31 @@ bool llvm::<u></u>isSafeToSpeculativelyExecute(<br>
default:<br>
return true;<br>
case Instruction::UDiv:<br>
- case Instruction::URem:<br>
- // x / y is undefined if y == 0, but calculations like x / 3 are safe.<br>
- return isKnownNonZero(Inst-><u></u>getOperand(1), TD);<br>
+ case Instruction::URem: {<br>
+ // x / y is undefined if y == 0.<br>
+ const APInt *V;<br>
+ if (match(Inst->getOperand(1), m_APInt(V)))<br>
+ return *V != 0;<br>
+ return false;<br>
+ }<br>
case Instruction::SDiv:<br>
case Instruction::SRem: {<br>
- Value *Op = Inst->getOperand(1);<br>
- // x / y is undefined if y == 0<br>
- if (!isKnownNonZero(Op, TD))<br>
- return false;<br>
- // x / y might be undefined if y == -1<br>
- unsigned BitWidth = getBitWidth(Op->getType(), TD);<br>
- if (BitWidth == 0)<br>
- return false;<br>
- APInt KnownZero(BitWidth, 0);<br>
- APInt KnownOne(BitWidth, 0);<br>
- computeKnownBits(Op, KnownZero, KnownOne, TD);<br>
- return !!KnownZero;<br>
+ // x / y is undefined if y == 0 or x == INT_MIN and y == -1<br>
+ const APInt *X, *Y;<br>
+ if (match(Inst->getOperand(1), m_APInt(Y))) {<br>
+ if (*Y != 0) {<br>
+ if (*Y == -1) {<br>
+ // The numerator can't be MinSignedValue if the denominator is -1.<br>
+ if (match(Inst->getOperand(0), m_APInt(X)))<br>
+ return !Y->isMinSignedValue();<br>
+ // The numerator *might* be MinSignedValue.<br>
+ return false;<br>
+ }<br>
+ // The denominator is not 0 or -1, it's safe to proceed.<br>
+ return true;<br>
+ }<br>
+ }<br>
+ return false;<br>
}<br>
case Instruction::Load: {<br>
const LoadInst *LI = cast<LoadInst>(Inst);<br>
<br>
Modified: llvm/trunk/test/Transforms/<u></u>LICM/speculate.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LICM/speculate.ll?rev=221318&r1=221317&r2=221318&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/test/<u></u>Transforms/LICM/speculate.ll?<u></u>rev=221318&r1=221317&r2=<u></u>221318&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/test/Transforms/<u></u>LICM/speculate.ll (original)<br>
+++ llvm/trunk/test/Transforms/<u></u>LICM/speculate.ll Tue Nov 4 17:49:08 2014<br>
@@ -3,12 +3,11 @@<br>
; UDiv is safe to speculate if the denominator is known non-zero.<br>
; CHECK-LABEL: @safe_udiv(<br>
-; CHECK: %div = udiv i64 %x, %or<br>
+; CHECK: %div = udiv i64 %x, 2<br>
; CHECK-NEXT: br label %for.body<br>
define void @safe_udiv(i64 %x, i64 %m, i64 %n, i32* %p, i64* %q) nounwind {<br>
entry:<br>
- %or = or i64 %m, 1<br>
br label %for.body<br>
for.body: ; preds = %entry, %for.inc<br>
@@ -19,7 +18,7 @@ for.body:<br>
br i1 %tobool, label %for.inc, label %if.then<br>
if.then: ; preds = %for.body<br>
- %div = udiv i64 %x, %or<br>
+ %div = udiv i64 %x, 2<br>
%arrayidx1 = getelementptr inbounds i64* %q, i64 %i.02<br>
store i64 %div, i64* %arrayidx1, align 8<br>
br label %for.inc<br>
@@ -69,13 +68,12 @@ for.end:<br>
; known to have at least one zero bit.<br>
; CHECK-LABEL: @safe_sdiv(<br>
-; CHECK: %div = sdiv i64 %x, %or<br>
+; CHECK: %div = sdiv i64 %x, 2<br>
; CHECK-NEXT: br label %for.body<br>
define void @safe_sdiv(i64 %x, i64 %m, i64 %n, i32* %p, i64* %q) nounwind {<br>
entry:<br>
%and = and i64 %m, -3<br>
- %or = or i64 %and, 1<br>
br label %for.body<br>
for.body: ; preds = %entry, %for.inc<br>
@@ -86,7 +84,7 @@ for.body:<br>
br i1 %tobool, label %for.inc, label %if.then<br>
if.then: ; preds = %for.body<br>
- %div = sdiv i64 %x, %or<br>
+ %div = sdiv i64 %x, 2<br>
%arrayidx1 = getelementptr inbounds i64* %q, i64 %i.02<br>
store i64 %div, i64* %arrayidx1, align 8<br>
br label %for.inc<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
</div></div></blockquote></div><br></div>