[PATCH] D57625: [TargetLibraryInfo] Update run time support for Windows

Tue Feb 5 04:11:11 PST 2019

mstorsjo added a comment.

In D57625#1384112 <https://reviews.llvm.org/D57625#1384112>, @evandro wrote:

> In D57625#1382005 <https://reviews.llvm.org/D57625#1382005>, @mstorsjo wrote:
>
> > (And it also works with mingw which normally uses an ancient msvcrt.)
>
>
> Indeed, this change only affects Windows proper, as both Cygwin and MinGW were already excluded from this selection of supported math functions.

Oh, right - I didn't check the patch that far. Ok, so that's not a concern then.

But the other point still stands; even if compiling LLVM requires VS 2015 or newer, the generated code can work with older MSVC runtimes as well. @rnk - is there any declared minimum for this? I'm using Clang with MSVC 2013 SDK/runtimes just fine in one setup at least (although this setup currently uses Clang/LLVM 4.0, I haven't recently retested it with latest). AFAIK, the exact version of MSVC that is targeted is kept available via Triplet::getEnvironmentVersion, so unless it's intended to drop support for all the older runtimes as well, this would have to check the version of it.

Other than that, the patch is pretty hard to follow - it would be easier split up in individual changes - from my reading through it so far, I can see 3-4 different kinds of changes. E.g. moving the x86 specific single precision function block, making some math functions available, marking some other functions as unavailable (copysignf etc).

================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:168
+    if (!is64) {
+      TLI.setUnavailable(LibFunc_acosf);
+      TLI.setUnavailable(LibFunc_asinf);
----------------
FWIW, `is64` isn't the exact right condition for this; these functions are available for ARM (32 bit) as well, it's only i386 which is the exception.

I see this if statement/block was moved, and it earlier used the condition `if (T.getArch() == Triple::x86)`, which is correct even when taking arm/aarch64 into consideration.

================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:225
     TLI.setUnavailable(LibFunc_acoshf);
     TLI.setUnavailable(LibFunc_acoshl);
+    // asinh
----------------
For ucrt (VS2015 and newer), all of acosh, acoshf, acoshl are available in the crt DLL at least.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57625