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

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a subscriber: dmajor.
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:
> > > > > 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.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:264
+    if (!hasPartialSP || !hasPartialC99)
+      TLI.setUnavailable(LibFunc_logbf);
+    TLI.setUnavailable(LibFunc_nearbyintf);
----------------
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`.


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

https://reviews.llvm.org/D57625