[llvm-dev] Speculative execution of FP divide Instructions - Call-Graph Simplify

[Samuel Antão via llvm-dev]

Thank you all for the responses!

I understand that FP exceptions are activated programatically and can be
disabled. I guess my point is that those divisions can have side effects
and the compiler can't prove they have not. I imagine that, from a user
viewpoint, if I write code like:

double a, b, c
...
if (a > b)
  c = a/b

thinking that my conditional also guards against divisions by zero, and
activate the exceptions exactly to assert that, transformations that
speculate the execution of the divisions won't serve me well. The whole
purpose of having FP exception support just vanishes.

I guess that at the end of the day this is about choosing what is a "good"
default behaviour. I don't feel strongly one way or the other, just thought
that this deserved some discussion.

I don't think this is what Andrew is trying to solve, is it? Exceptions can
be activated anywhere, not necessarily in the same compilation unit.

Thanks again!
Samuel

On Wed, Mar 15, 2017 at 3:22 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> [+current llvm-dev address]
>
>
>
> On 03/15/2017 09:23 AM, Hal Finkel wrote:
>
>> Hi Samuel,
>>
>> What are you taking about? ;)
>>
>> The only way to get a SIGFPE from a floating-point division by zero is to
>> modify the floating-point environment to enable those exceptions.  We don't
>> support that (*). In the default (supported) environment, floating point
>> division is well defined for all inputs (dividing by 0 gives inf, by NaN
>> gives, NaN, etc.).
>>
>> Regarding whether it makes sense to speculate, that's clearly a
>> target-dependent property. On some targets this makes sense (e.g. OOO cores
>> where divides have high, by hideable, latency) and on some targets it
>> really doesn't. If we're speculating these on targets where we shouldn't,
>> then we need to fix the cost model.
>>
>> (*) There's ongoing work to change that. Search the mailing list for
>> Andrew Kaylor.
>>
>>  -Hal
>>
>> On 03/15/2017 04:38 AM, Samuel Antão wrote:
>>
>>> Hi all,
>>>
>>> I came across an issue caused by the Call-Graph Simplify Pass. Here is a
>>> a small repro:
>>>
>>> ```
>>> define double @foo(double %a1, double %a2, double %a3) #0 {
>>> entry:
>>>   %a_mul = fmul double %a1, %a2
>>>   %a_cmp = fcmp ogt double %a3, %a_mul
>>>   br i1 %a_cmp, label %a.then, label %a.end
>>>
>>> a.then:
>>>   %a_div = fdiv double %a_mul, %a3
>>>   br label %a.end
>>>
>>> a.end:
>>>   %a_factor = phi double [ %a_div, %a.then ], [ 1.000000e+00, %entry ]
>>>   ret double %a_factor
>>> }
>>> ```
>>> Here, the conditional is guarding a possible division by zero. However
>>> if I run CGSimplify on this I get:
>>> ```
>>> define double @foo(double %a1, double %a2, double %a3)
>>> local_unnamed_addr #0 {
>>> entry:
>>>   %a_mul = fmul double %a1, %a2
>>>   %a_cmp = fcmp olt double %a_mul, %a3
>>>   %a_div = fdiv double %a_mul, %a3
>>>   %a_factor = select i1 %a_cmp, double %a_div, double 1.000000e+00
>>>   ret double %a_factor
>>> }
>>> ```
>>> This will cause a FP arithmetic exception, and the application will get
>>> a SIGFPE signal. The code that drives the change in the optimizer relies on
>>> `llvm::isSafeToSpeculativelyExecute` to decide whether the division
>>> should be performed speculatively. Right now, this function returns true.
>>> One could do something similar to integer divisions and add a case there
>>> (this solved the issue for me):
>>> ```
>>> diff --git a/lib/Analysis/ValueTracking.cpp
>>> b/lib/Analysis/ValueTracking.cpp
>>> index 1761dac..c61fefd 100644
>>> --- a/lib/Analysis/ValueTracking.cpp
>>> +++ b/lib/Analysis/ValueTracking.cpp
>>> @@ -3352,6 +3352,21 @@ bool llvm::isSafeToSpeculativelyExecute(const
>>> Value *V,
>>>      // The numerator *might* be MinSignedValue.
>>>      return false;
>>>    }
>>> +  case Instruction::FDiv:
>>> +  case Instruction::FRem:{
>>> +    const Value *Denominator = Inst->getOperand(1);
>>> +    // x / y is undefined if y == 0
>>> +    // The denominator is not a constant, so there is nothing we can do
>>> to prove
>>> +    // it is non-zero.
>>> +    if (auto *VV = dyn_cast<ConstantVector>(Denominator))
>>> +      Denominator = VV->getSplatValue();
>>> +    if (!isa<ConstantFP>(Denominator))
>>> +      return false;
>>> +    // The denominator is a zero constant - we can't speculate here.
>>> +    if (m_AnyZero().match(Denominator))
>>> +      return false;
>>> +    return true;
>>> +  }
>>>    case Instruction::Load: {
>>>      const LoadInst *LI = cast<LoadInst>(Inst);
>>>      if (!LI->isUnordered() ||
>>> ```
>>> I did my tests using a powerpc64le target, but I couldn't find any
>>> target specific login involved in this transform. In any case, I wanted to
>>> drop the questions:
>>>
>>> - is there any target that would benefit from speculative fp divisions?
>>> - is there any target for which fp division does not have side effects?
>>>
>>> If not, I can go ahead and post the patch above for review.
>>>
>>> Many thanks!
>>> Samuel
>>>
>>
>>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170315/91505b50/attachment.html>