[llvm] r221318 - Analysis: Make isSafeToSpeculativelyExecute fire less for divides
David Majnemer
david.majnemer at gmail.com
Wed Nov 5 18:28:24 PST 2014
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'.
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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141105/8e9d6ce5/attachment.html>
More information about the llvm-commits
mailing list