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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

Thanks, this looks much more readable!

> It's not clear to me that the version of the MSVCRT is handled by the triplet.

Hmm, indeed. The triplet can be used to set the MSVC compatibility version though, e.g. like `-target <arch>-windows-msvc19` for indicating MSVC 19 (aka 2015), which is the version that got UCRT which has got all these new functions.

I expected the reverse to be true as well, that the internal triplet gets populated with the version number, but it seems I might have misread the source. In Clang it's handled via code like `LangOpts.isCompatibleWithMSVC(LangOptions::MSVC2015)` and similar.

@rnk - do you know if it's possible to check the MSVC(RT) target version within LLVM somehow?



================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:168
+    if (!is64) {
+      TLI.setUnavailable(LibFunc_acosf);
+      TLI.setUnavailable(LibFunc_asinf);
----------------
evandro wrote:
> 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.
Right, IA64 probably isn't quite the same either - I normally only consider x86/x86_64/arm/arm64... This form certainly should be safe at least.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:225
     TLI.setUnavailable(LibFunc_acoshf);
     TLI.setUnavailable(LibFunc_acoshl);
+    // asinh
----------------
evandro wrote:
> 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.
Can you quote which part there says they're wrappers? I would at least expect e.g. `acoshf()` to be distinct from `acosh()`. Since long double and double are equivalent, I don't doubt that `acoshl()` is a wrapper around `acosh()` though. But in practice, even if e.g. the headers would do such redirecting, the MSVCRT (both DLL and import library) still provides all three function names, both plain, with `f` and `l` suffix.


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

https://reviews.llvm.org/D57625