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

  • 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

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

Can you elaborate on this? What do you mean by figuring out where the restrictions are needed? -Hal

Can you elaborate on this? What do you mean by figuring out where the restrictions are needed?

Sure. I’m just referring to having the front end support the FENV_ACCESS pragma (or whatever else) to decide for whatever scope it is translating whether or not it needs the constrained intrinsics. I found one of the places in the front end where it was emitting an fadd instruction, and I wrote some code of the form ‘if (true) emit the intrinsic instead’ to test out an end-to-end code path. So what’s missing is a replacement of that ‘if (true)’ with something that actually checks the compilation context to decide what should happen.

I might have been exaggerating a bit when I described this as ‘much harder’ but generating the intrinsic is fairly trivial.

-Andy

I think that you should handle this in the same way that we handle the FP_CONTRACT pragma. If you grep clang for fp_contract and FPContractable you should see how this works. Â -Hal