[llvm] r221318 - Analysis: Make isSafeToSpeculativelyExecute fire less for divides

Wed Nov 5 19:00:24 PST 2014

----- Original Message -----
> From: "David Majnemer" <david.majnemer at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>, "Philip Reames" <listmail at philipreames.com>
> Sent: Wednesday, November 5, 2014 8:45:58 PM
> Subject: Re: [llvm] r221318 - Analysis: Make isSafeToSpeculativelyExecute fire less for divides
> 
> On Wed, Nov 5, 2014 at 6:39 PM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> 
> ----- Original Message -----
> > From: "David Majnemer" < david.majnemer at gmail.com >
> > To: "Philip Reames" < listmail at philipreames.com >
> > Cc: "LLVM Commits" < llvm-commits at cs.uiuc.edu >
> > Sent: Wednesday, November 5, 2014 8:28:24 PM
> > Subject: Re: [llvm] r221318 - Analysis: Make
> > isSafeToSpeculativelyExecute fire less for divides
> > 
> > 
> > 
> > It's a correctness issue.
> > 
> > 
> > In general one cannot know whether or not a divide instruction is
> > safe to execute because isKnownNonZero returns true for poison.
> > 
> > 
> > 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.
> > 
> > 
> > For instructions which trap, we need a stronger predicate than
> > 'isKnownNonZero': we need a hypothetical 'isKnownNeverToBeUndef'.
> 
> Wait, why? If the program produced undefined behavior, then it can
> trap in response to that. No?
> 
> 
> 
> Let's consider a loop which will never iterate even once but contains
> a divide instruction whose operands are invariant of the loop.
> 
> 
> 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.
> 

Fair enough; I see your point.

 -Hal

> 
> 
> -Hal
> 
> 
> 
> > 
> > 
> > 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.
> > 
> > 
> > On Wed, Nov 5, 2014 at 5:05 PM, Philip Reames <
> > listmail at philipreames.com > wrote:
> > 
> > 
> > David,
> > 
> > Is this a correctness issue or a heuristic? The description doesn't
> > say.
> > 
> > I find this change potentially concerning since it will heavily
> > influence LICM behaviour.
> > 
> > Philip
> > 
> > 
> > 
> > On 11/04/2014 03:49 PM, David Majnemer wrote:
> > 
> > 
> > Author: majnemer
> > Date: Tue Nov 4 17:49:08 2014
> > New Revision: 221318
> > 
> > URL: http://llvm.org/viewvc/llvm- project?rev=221318&view=rev
> > Log:
> > Analysis: Make isSafeToSpeculativelyExecute fire less for divides
> > 
> > Divides and remainder operations do not behave like other
> > operations
> > when they are given poison: they turn into undefined behavior.
> > 
> > It's really hard to know if the operands going into a div are or
> > are
> > not
> > poison. Because of this, we should only choose to speculate if
> > there
> > are constant operands which we can easily reason about.
> > 
> > This fixes PR21412.
> > 
> > Modified:
> > llvm/trunk/lib/Analysis/ ValueTracking.cpp
> > llvm/trunk/test/Transforms/ LICM/speculate.ll
> > 
> > Modified: llvm/trunk/lib/Analysis/ ValueTracking.cpp
> > URL: http://llvm.org/viewvc/llvm- project/llvm/trunk/lib/
> > Analysis/ValueTracking.cpp? rev=221318&r1=221317&r2=
> > 221318&view=diff
> > ============================== ==============================
> > ==================
> > --- llvm/trunk/lib/Analysis/ ValueTracking.cpp (original)
> > +++ llvm/trunk/lib/Analysis/ ValueTracking.cpp Tue Nov 4 17:49:08
> 
> 
> > 2014
> > @@ -2549,23 +2549,31 @@ bool llvm:: isSafeToSpeculativelyExecute(
> > default:
> > return true;
> > case Instruction::UDiv:
> > - case Instruction::URem:
> > - // x / y is undefined if y == 0, but calculations like x / 3 are
> > safe.
> > - return isKnownNonZero(Inst-> getOperand(1), TD);
> > + case Instruction::URem: {
> > + // x / y is undefined if y == 0.
> > + const APInt *V;
> > + if (match(Inst->getOperand(1), m_APInt(V)))
> > + return *V != 0;
> > + return false;
> > + }
> > case Instruction::SDiv:
> > case Instruction::SRem: {
> > - Value *Op = Inst->getOperand(1);
> > - // x / y is undefined if y == 0
> > - if (!isKnownNonZero(Op, TD))
> > - return false;
> > - // x / y might be undefined if y == -1
> > - unsigned BitWidth = getBitWidth(Op->getType(), TD);
> > - if (BitWidth == 0)
> > - return false;
> > - APInt KnownZero(BitWidth, 0);
> > - APInt KnownOne(BitWidth, 0);
> > - computeKnownBits(Op, KnownZero, KnownOne, TD);
> > - return !!KnownZero;
> > + // x / y is undefined if y == 0 or x == INT_MIN and y == -1
> > + const APInt *X, *Y;
> > + if (match(Inst->getOperand(1), m_APInt(Y))) {
> > + if (*Y != 0) {
> > + if (*Y == -1) {
> > + // The numerator can't be MinSignedValue if the denominator is
> > -1.
> > + if (match(Inst->getOperand(0), m_APInt(X)))
> > + return !Y->isMinSignedValue();
> > + // The numerator *might* be MinSignedValue.
> > + return false;
> > + }
> > + // The denominator is not 0 or -1, it's safe to proceed.
> > + return true;
> > + }
> > + }
> > + return false;
> > }
> > case Instruction::Load: {
> > const LoadInst *LI = cast<LoadInst>(Inst);
> > 
> > Modified: llvm/trunk/test/Transforms/ LICM/speculate.ll
> > URL: http://llvm.org/viewvc/llvm- project/llvm/trunk/test/
> > Transforms/LICM/speculate.ll? rev=221318&r1=221317&r2=
> > 221318&view=diff
> > ============================== ==============================
> > ==================
> > --- llvm/trunk/test/Transforms/ LICM/speculate.ll (original)
> > +++ llvm/trunk/test/Transforms/ LICM/speculate.ll Tue Nov 4
> > 17:49:08
> 
> 
> > 2014
> > @@ -3,12 +3,11 @@
> > ; UDiv is safe to speculate if the denominator is known non-zero.
> > ; CHECK-LABEL: @safe_udiv(
> > -; CHECK: %div = udiv i64 %x, %or
> > +; CHECK: %div = udiv i64 %x, 2
> > ; CHECK-NEXT: br label %for.body
> > define void @safe_udiv(i64 %x, i64 %m, i64 %n, i32* %p, i64* %q)
> > nounwind {
> > entry:
> > - %or = or i64 %m, 1
> > br label %for.body
> > for.body: ; preds = %entry, %for.inc
> > @@ -19,7 +18,7 @@ for.body:
> > br i1 %tobool, label %for.inc, label %if.then
> > if.then: ; preds = %for.body
> > - %div = udiv i64 %x, %or
> > + %div = udiv i64 %x, 2
> > %arrayidx1 = getelementptr inbounds i64* %q, i64 %i.02
> > store i64 %div, i64* %arrayidx1, align 8
> > br label %for.inc
> > @@ -69,13 +68,12 @@ for.end:
> > ; known to have at least one zero bit.
> > ; CHECK-LABEL: @safe_sdiv(
> > -; CHECK: %div = sdiv i64 %x, %or
> > +; CHECK: %div = sdiv i64 %x, 2
> > ; CHECK-NEXT: br label %for.body
> > define void @safe_sdiv(i64 %x, i64 %m, i64 %n, i32* %p, i64* %q)
> > nounwind {
> > entry:
> > %and = and i64 %m, -3
> > - %or = or i64 %and, 1
> > br label %for.body
> > for.body: ; preds = %entry, %for.inc
> > @@ -86,7 +84,7 @@ for.body:
> > br i1 %tobool, label %for.inc, label %if.then
> > if.then: ; preds = %for.body
> > - %div = sdiv i64 %x, %or
> > + %div = sdiv i64 %x, 2
> > %arrayidx1 = getelementptr inbounds i64* %q, i64 %i.02
> > store i64 %div, i64* %arrayidx1, align 8
> > br label %for.inc
> > 
> > 
> > ______________________________ _________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/ mailman/listinfo/llvm-commits
> > 
> > 
> > 
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory