[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:264
+    if (!hasPartialSP || !hasPartialC99)
+      TLI.setUnavailable(LibFunc_logbf);
+    TLI.setUnavailable(LibFunc_nearbyintf);
----------------
evandro wrote:
> mstorsjo wrote:
> > 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");
> > ```
> I think that this gives me a plan of action: I'll commit this patch as is and commit another adding the remaining functions I find out after examining the information available in MinGW, keeping in mind that, to LLVM, MSVCRT and MinGW are different targets (specifically, MinGW using the GNU runtime).  This way, should any problem arise later, it should be easy to narrow it down and fix it.
> 
> Thank you so very much.  
Well, MSVC+MSVCRT and MinGW are different targets yes, but MinGW does try to describe the same actual DLLs from Microsoft.

A bit of backstory here: Traditionally, MinGW has linked to the old msvcrt.dll that is shipped with Windows (which is explicitly discouraged by Microsoft), in order to avoid relying on having to ship a CRT with the distributed apps. That DLL hasn't evolved much at all during the years and versions. In addition to this, MinGW does have listings of functions for the newer MSVC provided DLLs like msvcr100.dll, msvcr110.dll etc, but these aren't used by default. Recently though, mingw-w64 has gotten support for the new UCRT, and also support for setting this as the default version used.

So even if they're different targets in LLVM (and the MinGW target has to cope with the old discouraged and ancient msvcrt.dll), the MinGW def file for ucrtbase.dll and the other msvcr*.dll should be fairly accurate and corrent descriptions for what they contain even for a MSVC context. (There are a few MinGW specifics like having a few functions on their own added via a static library, but the def listings should be accurate despite this.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57625