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

No, `acosf` is one of those that the i386 library does not provide - it's part of the block of architecture specific functions regardless of msvcrt version above.

But for the ones that are added by `hasPartialC99` in your patch, let's look at e.g. `cbrt` - the suffixed forms `cbrtf` and `cbrtl` are also available. The documentation at https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/cbrt-cbrtf-cbrtl?view=vs-2015 say that the suffixed forms are available (the extra suffixless overloads only for C++), `corecrt_math.h` in the Win10 SDK contains this:
```
    _Check_return_ _ACRTIMP float     __cdecl cbrtf(_In_ float _X);
...
    _Check_return_ _ACRTIMP long double __cdecl cbrtl(_In_ long double _X);
```

And the import library, even for i386, contains this:
```
$ llvm-nm ucrt/x86/ucrt.lib | grep cbrt
00000000 T __imp__cbrt
00000000 T _cbrt
00000000 T __imp__cbrtf
00000000 T _cbrtf
00000000 T __imp__cbrtl
00000000 T _cbrtl
```

Given this, there is no doubt or inconsistency, all three references (docs, headers and import libs) agree that these functions are available in all three forms, `cbrt`, `cbrtf` and `cbrtl`. (Even though headers don't matter for what actually is available for linking.) Whether they internally call each other or not is irrelevant.

The same goes for all the functions in the `// Win32 does not fully support C99 math functions.` `if (!hasPartialC99)` block in the latest revision of your patch.

Now I've bothered you more than enough though; keeping it like this should be ok in the sense that it shouldn't break anything and I don't mind if you commit it in this form, even though I feel it still is inconsistent/incomplete.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:264
+    if (!hasPartialSP || !hasPartialC99)
+      TLI.setUnavailable(LibFunc_logbf);
+    TLI.setUnavailable(LibFunc_nearbyintf);
----------------
I believe this one doesn't need the `!hasPartialSP` condition. `logbf` is available for all architectures in the version where we set `hasPartialC99`.

(Also, what does `SP` stand for in that variable name?)

Changing it into ìf (!hasPartialSP && !hasPartialC99)` should be fine though, and conversely `if (hasPartialSP || hasPartialC99) TLI.setAvailableWithName(LibFunc_logbf, "_logbf");` below.


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

https://reviews.llvm.org/D57625