[PATCH] D129155: [InstCombine][SimplifyLibCalls] convert sqrt libcalls with "nnan" to sqrt intrinsics

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D129155#3631251 <https://reviews.llvm.org/D129155#3631251>, @efriedma wrote:

> I hate trying to reason about this stuff.
>
> The intrinsics for sqrt/sin/cos/pow/exp/exp2/log/log2/log10 are theoretically supposed to not write to errno, but in practice they do because we call the libc implementations.  So even though this is safe in theory, in practice it could cause a miscompile (if we hoist a sqrt operation past a load from errno).  Not sure what, if anything, we should do about this.

Ah, I didn't think of the sqrt call moving relative to other calls/accesses of errno. Are we already exposed to that problem in a bigger way when the front-end creates intrinsics too?
As an alternative, we could put this kind of check into PartiallyInlineLibCalls - that's where we create the extra call (that we then expect to be lowered to an instruction) after checking TTI->haveFastSqrt(). Or since we have TTI in AggressiveInstCombine, we could do the transform there. Do you see any potential miscompiles if we use that hook to guard the transform?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129155/new/

https://reviews.llvm.org/D129155