[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:
> > > > > > 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.
> No, you haven't bothered me at all.  I appreciate your input.
> 
> BTW, if you have a list of functions present in the i386 libraries and another in the, say, amd64 ones, as a proposal about what LLVM should know about the MSVC libraries, please do.
I guess the ultimate authority for what works linking wise, is the import lib and static crt lib files themselves. If you have access to them, `llvm-nm` on them gives a full list.

For a more readable reference, I'd recommend the mingw def files and wine spec files. They are based on dumping the list of exported functions from the DLLs, merged into one file for all architectures, with manual annotations about functions that only are available on specific architectures.

For the current CRT version that would be:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/lib-common/ucrtbase.def.in
https://source.winehq.org/git/wine.git/blob/957a1f0216995c14c3a3fe737358578a506af707:/dlls/ucrtbase/ucrtbase.spec

E.g. for the block of `if (!hasPartialSP)` above, for the `acosf` function, the mingw def file has got this:
```
F_NON_I386(acosf F_X86_ANY(DATA))
```
(You can ignore the `F_X86_ANY(DATA)` part, that's a mingw specific tweak not relevant to whether the function exists or not.)
Here this function is available for all architectues except i386. (mingw-w64 only supports i386, x86_64, arm, arm64, so it doesn't take any stance on e.g. IA64.)

Similarly the wine spec file shows this:
```
@ cdecl -arch=arm,x86_64,arm64 acosf(float) MSVCRT_acosf
```
That is, the function is explicitly available on these three architectures.


But then for the `logbf` case, things are actually different. The mingw def file contains this:
```
F_NON_I386(_logbf)
logbf
```
(These are symbols without the extra leading underscore which is prepended on cdecl functions on i386 though.)
The wine spec file:
```
@ cdecl -arch=arm,x86_64,arm64 _logbf(float) MSVCRT__logbf
@ stub logbf
```
This means that there's both a `_logbf` function (only on the three non-i386 arches) and a `logbf` function (on all arches). (The `stub` part only means that wine hasn't hooked it up to anything, probably because nobody has encountered a program using that function yet.)

If you want to look at what existed in older versions, pick e.g. `msvcr100` or `msvcr110`, because `msvcr120` actually already seems to have gotten most of the new functions that we're adding conditionally with `hasPartialC99`. For mingw-w64, those def files aren't merged into one common for all architectures, but are kept separately in the `lib32` and `lib64` directories next to `lib-common`.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:264
+    if (!hasPartialSP || !hasPartialC99)
+      TLI.setUnavailable(LibFunc_logbf);
+    TLI.setUnavailable(LibFunc_nearbyintf);
----------------
evandro wrote:
> mstorsjo wrote:
> > 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.
> `logbf` was available in amd64 even before C99 was supported.
> 
> `SP` stands for single precision, AKA, `float`.   I could rename it to `hasPartialFloat`.
Yes,  `logbf` was available for x86_64 before, but wih this change, it only becomes available for the modern CRT. That is, I suggest changing `||` in the condition to `&&` here. But as the `_logbf` name only exists for x86_64/arm/aarch64, I would suggest this:

```
// Unavailable for i386 on older crt versions, available for i386 on modern crts and on all crt versions for other architectures.
if (!hasPartialSP && !hasPartialC99)
  TLI.setUnavailable(LibFunc_logbf);
// On non-i386 versions, this function always exists, as _logbf.
// Modern crt versions also provide it under the name logbf, for all archs.
if (hasPartialSP)
 TLI.setAvailableWithName(LibFunc_logbf, "_logbf");
```


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

https://reviews.llvm.org/D57625