Page MenuHomePhabricator

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

Authored by spatel on Nov 5 2017, 8:53 AM.

Details

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.

Diff Detail

Event Timeline

spatel created this revision.Nov 5 2017, 8:53 AM
efriedma accepted this revision.Nov 6 2017, 12:16 PM
efriedma added a subscriber: arsenm.

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.

This revision is now accepted and ready to land.Nov 6 2017, 12:16 PM

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.

This revision was automatically updated to reflect the committed changes.