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

Tue Feb 5 09:34:14 PST 2019

evandro marked 4 inline comments as done.
evandro added a comment.

In D57625#1384950 <https://reviews.llvm.org/D57625#1384950>, @mstorsjo wrote:

> 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.

It's not clear to me that the version of the MSVCRT is handled by the triplet.  However, when I worked at AMD, we worked with MS to get them more math functions into it for x86-64, so it's probably been the case since then.  However, after 15 years or so, my recollection of the details is fuzzy.

Again, I don't work on Windows, so I'd appreciate any help sorting out the legality of these changes.

================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:168
+    if (!is64) {
+      TLI.setUnavailable(LibFunc_acosf);
+      TLI.setUnavailable(LibFunc_asinf);
----------------
mstorsjo wrote:
> 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.
It's true that ARM is also the case.  However, there are some tidbits for IA64 in MSVCRT, so I don't think that excepting i386 is safe.

================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:225
     TLI.setUnavailable(LibFunc_acoshf);
     TLI.setUnavailable(LibFunc_acoshl);
+    // asinh
----------------
mstorsjo wrote:
> For ucrt (VS2015 and newer), all of acosh, acoshf, acoshl are available in the crt DLL at least.
As far as I can tell, `acoshf()` and `acoshl()` are wrappers around `acosh()`.   Likewise for other math functions. This is confirmed by the documentation at https://is.gd/W5fT9r.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57625