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

[Evandro Menezes via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:225
     TLI.setUnavailable(LibFunc_acoshf);
     TLI.setUnavailable(LibFunc_acoshl);
+    // asinh
----------------
mstorsjo wrote:
> 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.
It's true that that statement is explicitly about function overloading in C++, but I suspected that there was more to it.  Many math functions are defined as macros around the `double` version.  Even then the symbol for other versions are defined in the library, but some just replicate the macro, wrapping around the `double` version with parameters and result conversion.  

Now, I didn't do a thorough inventory of the libraries, just sampled a handful of functions.  In the header file `math.h` it's more straightforward to figure out.

In the case of `acosf`, we find:

  #if defined _M_X64 || defined _M_ARM || defined _M_ARM64

    _Check_return_ _ACRTIMP float __cdecl acosf(_In_ float _X);
    ...

  #else

    _Check_return_ __inline float __CRTDECL acosf(_In_ float _X)
    {
        return (float)acos(_X);
    }
    ...

  #endif

Even though the i386 library also provides the symbol `acosf`, but for a wrapper equivalent to this macro.

Bottom line is that I could not find a straightforward documentation from MS and the best that I could confidently come up with was the result of reverse engineering the files that accompany VS.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:182
+    // Win32 does not support float math functions, in general.
+    if (!isPartialC99) {
       TLI.setUnavailable(LibFunc_acosf);
----------------
mstorsjo wrote:
> 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).
OK, I think that I understand what you mean.  Stand by.


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

https://reviews.llvm.org/D57625