[PATCH] D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally created this revision.
cameron.mcinally added reviewers: uweigand, craig.topper, andrew.w.kaylor, hfinkel, kpn.
Herald added a subscriber: llvm-commits.

This isn't so much a patch as it is a tool for discussion. We shouldn't need constrained intrinsics for copysign/fabs/fneg since they are bitwise operations (assuming the intrinsics map to libm functions). They never trap and are not influenced by the rounding mode.

That said, there are cases where copysign/etc optimizations can result in trap-unsafe code. E.g.:

define double @safe(float %a, double %b) "unsafe-fp-math"="false" {

  %r1 = call float @llvm.copysign.f32(float 1.000000e+00, float %a)
  %r2 = fpext float %r1 to double
  %r3 = call double @llvm.copysign.f64(double %b, double %r2)
  ret double %r3

}

In this example, the 1st copysign is sanitizing a potential SNaN so that the fpext does not trap on it. LLVM will currently optimize this sequence to remove the 1st copysign, which may lead to a FE_INVALID trap on the fpext.

As long as we add constrained intrinsics for the fpext (and friends), we won't be able to optimize the copysign in an unsafe way. So, I think we're good with just that.

In conclusion, we don't need constrained intrinsics for copysign/fabs/fneg, but we should be wary of how they're optimized in the presence of constrained converts.

Does this sound correct to everyone? Did I miss anything?


Repository:
  rL LLVM

https://reviews.llvm.org/D50913

Files:
  include/llvm/IR/Intrinsics.td


Index: include/llvm/IR/Intrinsics.td
===================================================================
--- include/llvm/IR/Intrinsics.td
+++ include/llvm/IR/Intrinsics.td
@@ -559,7 +559,7 @@
                                                            llvm_metadata_ty ]>;
 }
 // FIXME: Add intrinsics for fcmp, fptrunc, fpext, fptoui and fptosi.
-// FIXME: Add intrinsics for fabs, copysign, floor, ceil, trunc and round?
+// FIXME: Add intrinsics for floor, ceil, trunc and round?
 
 
 //===------------------------- Expect Intrinsics --------------------------===//


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50913.161268.patch
Type: text/x-patch
Size: 570 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180817/2a4bc682/attachment.bin>