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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:225
     TLI.setUnavailable(LibFunc_acoshf);
     TLI.setUnavailable(LibFunc_acoshl);
+    // asinh
----------------
evandro wrote:
> mstorsjo wrote:
> > 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.
> This is what the document says:
> 
> > When you use C++, you can call overloads of *acosh* that take and return *float* or *long* double values. In a C program, *acosh* always takes and returns *double*.
> 
> Disassembling the DLL, it's not clear that the `float` version just calls the `double` version after the proper conversions, though.  
> 
> We're dealing with very poor documentation that doesn't quite match the header files that don't quite match the object file.
> 
> I'm making a judgment call for the the least common denominator: the poor documentation.
> 
> 
> This is what the document says:
> 
> > When you use C++, you can call overloads of *acosh* that take and return *float* or *long* double values. In a C program, *acosh* always takes and returns *double*.

This only says that while in C mode, you have the three distinct function names `acosh`, `acoshf` and `acoshl`, you can also use the plain unsuffixed function name `acosh` in C++ mode, as C++ allows overloads. This doesn't imply that the suffixed versions like `acoshf` are unavailable in C++ mode - they certainly are available as well.

> Disassembling the DLL, it's not clear that the float version just calls the double version after the proper conversions, though.
> 
> We're dealing with very poor documentation that doesn't quite match the header files that don't quite match the object file.
> 
> I'm making a judgment call for the the least common denominator: the poor documentation.

I don't see why one would have to disassemble the DLL to figure out what calls what or what C++ overloads exist matter at all.

At this level, within LLVM, we are past the frontend languages and don't know or care about potential redirections within `math.h`, and language differences between C and C++ and others are irrelevant as we are on the object file level.

The only thing matters is "can we assume that a definition of a function named `acoshf` will exist elsewhere at link time". And the answer to that is yes. Whether the implementation of that function simply internally calls `acosh` or not is irrelevant for LLVM - that doesn't disqualify its use.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:182
+    // Win32 does not support float math functions, in general.
+    if (!isPartialC99) {
       TLI.setUnavailable(LibFunc_acosf);
----------------
This changes behaviour for old MSVCRT versions as well - you're hiding many orthogonal issues under the same `isPartialC99` variable.

The functions here, e.g. `acosf`, does exist even in very old MSVCRT versions, but only on x86_64, arm and arm64, not on i386. `acosf` does not exist as a function in `libcmt.lib` or `msvcrt.lib` (or `libucrt.lib` or `ucrt.lib` for modern versions) for i386, not even for the latest versions of MSVC.

After this change, `acosf` and the others in this if statement would become unavailable for x86_64 for MSVC versions prior to 19, even though we earlier allowed and used that function for any MSVCRT version, on x86_64.

So there's two orthogonal issues at play here:

1) Some functions (`f`-suffixed) don't exist on some architectures, but exist on others. Regardless of MSVCRT version.

2) Some functions (`acosh`, `rint`, etc) appeared in some MSVCRT version (MSVC 19 is a good and safe threshold for this even though they might have appeared earlier) but were missing before that, regardless of architecture. And once these appeared, they appeared with all three forms (`rint`, `rintf` and `rintl`) at once, on all architectures (even if the `l` suffixed version is pointless on MSVC as long double == double).


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

https://reviews.llvm.org/D57625