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

[Philip Reames]

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