llvm dev - Mar 2017 - Speculative execution of FP divide Instructions - Call-Graph Simplify

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!
Sam
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170315/6d7bd2b9/attachment.html>

> - is there any target for which fp division does not have side effects?
Yes - all of them. This goes back to the fact that clang/llvm do not
support changing the FPENV:
https://bugs.llvm.org/show_bug.cgi?id=8100

There has been some effort to change that recently, so maybe this is
(partly) fixed? (cc'ing Andy Kaylor)

These reports may also provide info / answers / work-arounds:
https://bugs.llvm.org/show_bug.cgi?id=6050
https://bugs.llvm.org/show_bug.cgi?id=24343
https://bugs.llvm.org/show_bug.cgi?id=24818


On Wed, Mar 15, 2017 at 3:41 AM, Samuel Antão via llvm-dev <
llvm-dev at lists.llvm.org> 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.c
> pp
> 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!
> Sam
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170315/ed01cd79/attachment.html>

It’s true, I am working on this.  I have committed the initial patch to add
constrained intrinsics for the basic FP operations.  This has the desired effect
of preventing optimizations that would violate strict FP semantics with regard
to rounding mode and exception status, but it also prevents many safe
optimizations.  Later this year I’ll be going through the code base and trying
to teach various optimizations to handle the constrained intrinsics safely.

Nothing has been added to clang yet to generate these intrinsics, though I have
some rough code to do so that’s just waiting for an implementation of the much
harder task of figuring out when such restrictions are needed.  If anyone did
have a front end that generated these intrinsics, everything is in place to get
them through to code generation (though they currently become at least
theoretically unrestricted again after ISel).  I have an experimental pass that
converts all basic FP operations to the constrained versions and I’ve been able
to successfully compile and run real-world FP-using applications with that pass
in place.

I’m currently working on a patch to add constrained versions of the standard
library FP intrinsics (sin, cos, pow, etc.).

If anyone is interested in getting pieces of the work-in-progress I’ve mentioned
above to test drive, let me know.  I’d be happy to share.

-Andy


From: Sanjay Patel [mailto:spatel at rotateright.com]
Sent: Wednesday, March 15, 2017 8:16 AM
To: Samuel Antão <samuelfantao at gmail.com>; Kaylor, Andrew
<andrew.kaylor at intel.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>; acjacob at us.ibm.com
Subject: Re: [llvm-dev] Speculative execution of FP divide Instructions -
Call-Graph Simplify
> - is there any target for which fp division does not have side effects?
Yes - all of them. This goes back to the fact that clang/llvm do not support
changing the FPENV:
https://bugs.llvm.org/show_bug.cgi?id=8100
There has been some effort to change that recently, so maybe this is (partly)
fixed? (cc'ing Andy Kaylor)
These reports may also provide info / answers / work-arounds:
https://bugs.llvm.org/show_bug.cgi?id=6050
https://bugs.llvm.org/show_bug.cgi?id=24343
https://bugs.llvm.org/show_bug.cgi?id=24818

On Wed, Mar 15, 2017 at 3:41 AM, Samuel Antão via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> 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!
Sam

_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170315/cd00a6a1/attachment.html>