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

[spatel]

Summary

As discussed in D39204, this is effectively a revert of 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 D39611. That would create the sqrt intrinsic before we ever reach this place in LLVM.

[efriedma]

LGTM.

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?

I'm not sure how the C standard fits into this? "-ffinite-math-only" mode intentionally doesn't conform to the C standard.

[spatel]

In D39642#917009, @efriedma wrote:

I'm not sure how the C standard fits into this? "-ffinite-math-only" mode intentionally doesn't conform to the C standard.

It's moot after this fix, but I was imagining a strict-mode case where we somehow determined that the sqrt does not have nan input or output.

define double @foo(double %x)  {
entry:
  %cmp.i = fcmp ord double %x, 0.0
  br i1 %cmp.i, label %no_nan_input, label %end

no_nan_input:  
  %sqrt = tail call double @sqrt(double %x) #2   <--- some pass can mark this call with 'nnan' if it knows the libcall won't return nan?
  br label %return

end:  
  %r = phi double [ %sqrt, %no_nan_input ], [ 1.0, %entry ]
  ret double %r
}

So if that sqrt got marked with nnan, the vectorizers or whoever else is using getIntrinsicForCallSite() would convert the libcall to the intrinsic, but that wouldn't be correct if %x is a negative number? We would fail to set errno when we should have.

lib/Analysis/ValueTracking.cpp

case LibFunc_roundl:
     return Intrinsic::round;
   case LibFunc_pow:
   case LibFunc_powf:
   case LibFunc_powl:
     return Intrinsic::pow;
   case LibFunc_sqrt:
   case LibFunc_sqrtf:
   case LibFunc_sqrtl:
-    if (ICS->hasNoNaNs())
-      return Intrinsic::sqrt;
-    return Intrinsic::not_intrinsic;
   }
 
   return Intrinsic::not_intrinsic;
 }
 
 /// Return true if we can prove that the specified FP value is never equal to
 /// -0.0.
 ///

test/Transforms/SLPVectorizer/X86/call.ll

;
   %exp1 = tail call double @exp2(double %a0) nounwind readnone
   %exp2 = tail call double @exp2(double %a1) nounwind readnone
   store double %exp1, double* %b, align 8
   %idx2 = getelementptr inbounds double, double* %b, i64 1
   store double %exp2, double* %idx2, align 8
   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
   ret void
 }
 
 ; Negative test case
 define void @round_custom(i64* %a, i64* %b) {
 ; CHECK-LABEL: @round_custom(
 ; 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
 ; CHECK-NEXT:    ret void
 ;
   %a0 = load i64, i64* %a, align 8
   %idx1 = getelementptr inbounds i64, i64* %a, i64 1
   %a1 = load i64, i64* %idx1, align 8