[PATCH] D39642: [ValueTracking] readnone is a requirement for converting sqrt to llvm.sqrt; nnan is not

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
Herald added a subscriber: mcrosier.

As discussed in https://reviews.llvm.org/D39204, this is effectively a revert of https://reviews.llvm.org/rL265521 which required nnan to vectorize sqrt libcalls based on the old LangRef definition of llvm.sqrt.

We have the right check to know that errno is not set:

  if (!ICS.onlyReadsMemory())

...ahead of the switch.

But after reviewing the C standard text, I'm not sure the previous behavior was correct in the first place. If "implementation-defined" is actually defined by the platform, then a compiler can't assume that value (is nan) as we were doing?

This will solve https://bugs.llvm.org/show_bug.cgi?id=27435 although I'm still planning to make clang changes for intrinsics after https://reviews.llvm.org/D39611. That would create the sqrt intrinsic before we ever reach this place in LLVM.


https://reviews.llvm.org/D39642

Files:
  lib/Analysis/ValueTracking.cpp
  test/Transforms/SLPVectorizer/X86/call.ll


Index: test/Transforms/SLPVectorizer/X86/call.ll
===================================================================
--- test/Transforms/SLPVectorizer/X86/call.ll
+++ test/Transforms/SLPVectorizer/X86/call.ll
@@ -91,20 +91,51 @@
   ret void
 }
 
-define void @sqrt_libm(double* %a, double* %b) {
-; CHECK-LABEL: @sqrt_libm(
+; No fast-math-flags are required to convert sqrt library calls to an intrinsic. 
+; We just need to know that errno is not set (readnone).
+
+define void @sqrt_libm_no_errno(double* %a, double* %b) {
+; CHECK-LABEL: @sqrt_libm_no_errno(
 ; CHECK-NEXT:    [[TMP1:%.*]] = bitcast double* %a to <2 x double>*
 ; CHECK-NEXT:    [[TMP2:%.*]] = load <2 x double>, <2 x double>* [[TMP1]], align 8
-; CHECK-NEXT:    [[TMP3:%.*]] = call nnan <2 x double> @llvm.sqrt.v2f64(<2 x double> [[TMP2]])
+; CHECK-NEXT:    [[TMP3:%.*]] = call <2 x double> @llvm.sqrt.v2f64(<2 x double> [[TMP2]])
 ; CHECK-NEXT:    [[TMP4:%.*]] = bitcast double* %b to <2 x double>*
 ; CHECK-NEXT:    store <2 x double> [[TMP3]], <2 x double>* [[TMP4]], align 8
 ; CHECK-NEXT:    ret void
 ;
   %a0 = load double, double* %a, align 8
   %idx1 = getelementptr inbounds double, double* %a, i64 1
   %a1 = load double, double* %idx1, align 8
-  %sqrt1 = tail call nnan double @sqrt(double %a0) nounwind readnone
-  %sqrt2 = tail call nnan double @sqrt(double %a1) nounwind readnone
+  %sqrt1 = tail call double @sqrt(double %a0) nounwind readnone
+  %sqrt2 = tail call double @sqrt(double %a1) nounwind readnone
+  store double %sqrt1, double* %b, align 8
+  %idx2 = getelementptr inbounds double, double* %b, i64 1
+  store double %sqrt2, double* %idx2, align 8
+  ret void
+}
+
+; The sqrt intrinsic does not set errno, but a non-constant sqrt call might, so this can't vectorize.
+; The nnan on the call does not matter because there's no guarantee in the C standard that a negative
+; input would result in a nan output ("On a domain error, the function returns an 
+; implementation-defined value.")
+
+define void @sqrt_libm_errno(double* %a, double* %b) {
+; CHECK-LABEL: @sqrt_libm_errno(
+; CHECK-NEXT:    [[A0:%.*]] = load double, double* %a, align 8
+; CHECK-NEXT:    [[IDX1:%.*]] = getelementptr inbounds double, double* %a, i64 1
+; CHECK-NEXT:    [[A1:%.*]] = load double, double* [[IDX1]], align 8
+; CHECK-NEXT:    [[SQRT1:%.*]] = tail call nnan double @sqrt(double [[A0]]) #2
+; CHECK-NEXT:    [[SQRT2:%.*]] = tail call nnan double @sqrt(double [[A1]]) #2
+; CHECK-NEXT:    store double [[SQRT1]], double* %b, align 8
+; CHECK-NEXT:    [[IDX2:%.*]] = getelementptr inbounds double, double* %b, i64 1
+; CHECK-NEXT:    store double [[SQRT2]], double* [[IDX2]], align 8
+; CHECK-NEXT:    ret void
+;
+  %a0 = load double, double* %a, align 8
+  %idx1 = getelementptr inbounds double, double* %a, i64 1
+  %a1 = load double, double* %idx1, align 8
+  %sqrt1 = tail call nnan double @sqrt(double %a0) nounwind
+  %sqrt2 = tail call nnan double @sqrt(double %a1) nounwind
   store double %sqrt1, double* %b, align 8
   %idx2 = getelementptr inbounds double, double* %b, i64 1
   store double %sqrt2, double* %idx2, align 8
@@ -117,8 +148,8 @@
 ; CHECK-NEXT:    [[A0:%.*]] = load i64, i64* %a, align 8
 ; CHECK-NEXT:    [[IDX1:%.*]] = getelementptr inbounds i64, i64* %a, i64 1
 ; CHECK-NEXT:    [[A1:%.*]] = load i64, i64* [[IDX1]], align 8
-; CHECK-NEXT:    [[ROUND1:%.*]] = tail call i64 @round(i64 [[A0]]) #2
-; CHECK-NEXT:    [[ROUND2:%.*]] = tail call i64 @round(i64 [[A1]]) #2
+; CHECK-NEXT:    [[ROUND1:%.*]] = tail call i64 @round(i64 [[A0]]) #3
+; CHECK-NEXT:    [[ROUND2:%.*]] = tail call i64 @round(i64 [[A1]]) #3
 ; CHECK-NEXT:    store i64 [[ROUND1]], i64* %b, align 8
 ; CHECK-NEXT:    [[IDX2:%.*]] = getelementptr inbounds i64, i64* %b, i64 1
 ; CHECK-NEXT:    store i64 [[ROUND2]], i64* [[IDX2]], align 8
Index: lib/Analysis/ValueTracking.cpp
===================================================================
--- lib/Analysis/ValueTracking.cpp
+++ lib/Analysis/ValueTracking.cpp
@@ -2579,9 +2579,7 @@
   case LibFunc_sqrt:
   case LibFunc_sqrtf:
   case LibFunc_sqrtl:
-    if (ICS->hasNoNaNs())
-      return Intrinsic::sqrt;
-    return Intrinsic::not_intrinsic;
+    return Intrinsic::sqrt;
   }
 
   return Intrinsic::not_intrinsic;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39642.121628.patch
Type: text/x-patch
Size: 4253 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171105/878d621d/attachment.bin>