<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 5, 2014 at 6:39 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> To: "Philip Reames" <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
> Cc: "LLVM Commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> Sent: Wednesday, November 5, 2014 8:28:24 PM<br>
> Subject: Re: [llvm] r221318 - Analysis: Make isSafeToSpeculativelyExecute fire        less for divides<br>
><br>
><br>
><br>
> It's a correctness issue.<br>
><br>
><br>
> In general one cannot know whether or not a divide instruction is<br>
> safe to execute because isKnownNonZero returns true for poison.<br>
><br>
><br>
> It is possible for the right hand side of the divide to always be<br>
> poison in a valid program so long as the divide cannot ever be<br>
> executed.<br>
><br>
><br>
> For instructions which trap, we need a stronger predicate than<br>
> 'isKnownNonZero': we need a hypothetical 'isKnownNeverToBeUndef'.<br>
<br>
</span>Wait, why? If the program produced undefined behavior, then it can trap in response to that. No?<br></blockquote><div><br></div><div>Let's consider a loop which will never iterate even once but contains a divide instruction whose operands are invariant of the loop.</div><div><br></div><div>The behavior of this program is well defined because the divide cannot execute.  Hoisting the divide might turn a well-formed program into an ill-formed program if it turns out the dividend is poison.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 -Hal<br>
<div><div class="h5"><br>
><br>
><br>
> I doubt a correct implementation of isKnownNeverToBeUndef will ever<br>
> fire because it must be incredibly conservative: function arguments<br>
> might silently cary poison in them making them unsafe.<br>
><br>
><br>
> On Wed, Nov 5, 2014 at 5:05 PM, Philip Reames <<br>
> <a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a> > wrote:<br>
><br>
><br>
> David,<br>
><br>
> Is this a correctness issue or a heuristic? The description doesn't<br>
> say.<br>
><br>
> I find this change potentially concerning since it will heavily<br>
> influence LICM behaviour.<br>
><br>
> Philip<br>
><br>
><br>
><br>
> On 11/04/2014 03:49 PM, David Majnemer wrote:<br>
><br>
><br>
> 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-" target="_blank">http://llvm.org/viewvc/llvm-</a> project?rev=221318&view=rev<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<br>
> 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/ ValueTracking.cpp<br>
> llvm/trunk/test/Transforms/ LICM/speculate.ll<br>
><br>
> Modified: llvm/trunk/lib/Analysis/ ValueTracking.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a> project/llvm/trunk/lib/<br>
> Analysis/ValueTracking.cpp? rev=221318&r1=221317&r2=<br>
> 221318&view=diff<br>
> ============================== ==============================<br>
> ==================<br>
> --- llvm/trunk/lib/Analysis/ ValueTracking.cpp (original)<br>
</div></div>> +++ llvm/trunk/lib/Analysis/ ValueTracking.cpp Tue Nov 4 17:49:08<br>
<div><div class="h5">> 2014<br>
> @@ -2549,23 +2549,31 @@ bool llvm:: 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<br>
> safe.<br>
> - return isKnownNonZero(Inst-> 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/ LICM/speculate.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a> project/llvm/trunk/test/<br>
> Transforms/LICM/speculate.ll? rev=221318&r1=221317&r2=<br>
> 221318&view=diff<br>
> ============================== ==============================<br>
> ==================<br>
> --- llvm/trunk/test/Transforms/ LICM/speculate.ll (original)<br>
</div></div>> +++ llvm/trunk/test/Transforms/ LICM/speculate.ll Tue Nov 4 17:49:08<br>
<div class="HOEnZb"><div class="h5">> 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)<br>
> 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)<br>
> 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>
> ______________________________ _________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/" target="_blank">http://lists.cs.uiuc.edu/</a> mailman/listinfo/llvm-commits<br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>