Page MenuHomePhabricator

[TargetLibraryInfo] Update run time support for Windows
ClosedPublic

Authored by evandro on Feb 1 2019, 3:08 PM.

Details

Diff Detail

Event Timeline

evandro created this revision.Feb 1 2019, 3:08 PM
mstorsjo added a reviewer: rnk.Feb 2 2019, 4:08 AM

Even if a specific version of MSVC is required to compile LLVM, code generated by LLVM can run on a wider range of msvcrt versions. (And it also works with mingw which normally uses an ancient msvcrt.)

I didn't look closely at the change itself yet.

(And it also works with mingw which normally uses an ancient msvcrt.)

Indeed, this change only affects Windows proper, as both Cygwin and MinGW were already excluded from this selection of supported math functions.

(And it also works with mingw which normally uses an ancient msvcrt.)

Indeed, this change only affects Windows proper, as both Cygwin and MinGW were already excluded from this selection of supported math functions.

Oh, right - I didn't check the patch that far. Ok, so that's not a concern then.

But the other point still stands; even if compiling LLVM requires VS 2015 or newer, the generated code can work with older MSVC runtimes as well. @rnk - is there any declared minimum for this? I'm using Clang with MSVC 2013 SDK/runtimes just fine in one setup at least (although this setup currently uses Clang/LLVM 4.0, I haven't recently retested it with latest). AFAIK, the exact version of MSVC that is targeted is kept available via Triplet::getEnvironmentVersion, so unless it's intended to drop support for all the older runtimes as well, this would have to check the version of it.

Other than that, the patch is pretty hard to follow - it would be easier split up in individual changes - from my reading through it so far, I can see 3-4 different kinds of changes. E.g. moving the x86 specific single precision function block, making some math functions available, marking some other functions as unavailable (copysignf etc).

llvm/lib/Analysis/TargetLibraryInfo.cpp
168

FWIW, is64 isn't the exact right condition for this; these functions are available for ARM (32 bit) as well, it's only i386 which is the exception.

I see this if statement/block was moved, and it earlier used the condition if (T.getArch() == Triple::x86), which is correct even when taking arm/aarch64 into consideration.

225

For ucrt (VS2015 and newer), all of acosh, acoshf, acoshl are available in the crt DLL at least.

evandro marked 4 inline comments as done.Feb 5 2019, 9:33 AM

But the other point still stands; even if compiling LLVM requires VS 2015 or newer, the generated code can work with older MSVC runtimes as well. @rnk - is there any declared minimum for this? I'm using Clang with MSVC 2013 SDK/runtimes just fine in one setup at least (although this setup currently uses Clang/LLVM 4.0, I haven't recently retested it with latest). AFAIK, the exact version of MSVC that is targeted is kept available via Triplet::getEnvironmentVersion, so unless it's intended to drop support for all the older runtimes as well, this would have to check the version of it.

It's not clear to me that the version of the MSVCRT is handled by the triplet. However, when I worked at AMD, we worked with MS to get them more math functions into it for x86-64, so it's probably been the case since then. However, after 15 years or so, my recollection of the details is fuzzy.

Again, I don't work on Windows, so I'd appreciate any help sorting out the legality of these changes.

llvm/lib/Analysis/TargetLibraryInfo.cpp
168

It's true that ARM is also the case. However, there are some tidbits for IA64 in MSVCRT, so I don't think that excepting i386 is safe.

225

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.

evandro updated this revision to Diff 185336.Feb 5 2019, 9:45 AM
evandro marked 2 inline comments as done.

Include ARM.

rnk added a comment.Feb 5 2019, 2:16 PM

Do we know when most of these were added? Did the all arrive in more or less the same version? It would be nice to make this conditional on a new enough environment version. That gives users a way to indicate that they are targetting an old CRT if they need to.

mstorsjo marked an inline comment as done.Feb 5 2019, 2:23 PM

Thanks, this looks much more readable!

It's not clear to me that the version of the MSVCRT is handled by the triplet.

Hmm, indeed. The triplet can be used to set the MSVC compatibility version though, e.g. like -target <arch>-windows-msvc19 for indicating MSVC 19 (aka 2015), which is the version that got UCRT which has got all these new functions.

I expected the reverse to be true as well, that the internal triplet gets populated with the version number, but it seems I might have misread the source. In Clang it's handled via code like LangOpts.isCompatibleWithMSVC(LangOptions::MSVC2015) and similar.

@rnk - do you know if it's possible to check the MSVC(RT) target version within LLVM somehow?

llvm/lib/Analysis/TargetLibraryInfo.cpp
168

Right, IA64 probably isn't quite the same either - I normally only consider x86/x86_64/arm/arm64... This form certainly should be safe at least.

225

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.

In D57625#1386061, @rnk wrote:

Do we know when most of these were added? Did the all arrive in more or less the same version? It would be nice to make this conditional on a new enough environment version. That gives users a way to indicate that they are targetting an old CRT if they need to.

They're certainly available since UCRT, but msvcr120.dll (MSVC 2013, aka 18.00) also seems to have at least rint, round, cbrt, exp2. (I'm checking https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/lib64/msvcr120.def.in.)

rnk added a comment.Feb 5 2019, 2:57 PM

They're certainly available since UCRT, but msvcr120.dll (MSVC 2013, aka 18.00) also seems to have at least rint, round, cbrt, exp2. (I'm checking https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/lib64/msvcr120.def.in.)

I don't think we need to go the extra mile to provide rint to users targeting 2013. We can reduce it to just pre-2015 and post, as is done in this patch.

llvm/lib/Analysis/TargetLibraryInfo.cpp
165

I think you can use T.getEnvironmentVersion() here to get the MSVC version. Maybe make it conditional on isWindowsMSVCEnvironment(). If there is no version or it isn't an MSVC environment, just assume full C99 support is available (i.e. the UCRT is in use).

evandro marked 2 inline comments as done.Feb 5 2019, 3:03 PM
In D57625#1386061, @rnk wrote:

Do we know when most of these were added? Did the all arrive in more or less the same version? It would be nice to make this conditional on a new enough environment version. That gives users a way to indicate that they are targetting an old CRT if they need to.

Goodness, I wrote this document (https://is.gd/vQYYpY) in 2004 about VS6! Just don't ask me any question about it, as it was in another life.

llvm/lib/Analysis/TargetLibraryInfo.cpp
225

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.

evandro marked 2 inline comments as done.Feb 5 2019, 3:15 PM
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
165

What's the correlation between MSVCRT major version and VS release year?

evandro marked an inline comment as done.Feb 5 2019, 3:17 PM
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
165

FWIW, LLVM seems to be aware only of major versions 18 and 19, according to test cases.

rnk added inline comments.Feb 5 2019, 3:18 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
165

In this case, I think it's VS 2015 == VC 19.

evandro updated this revision to Diff 185452.Feb 5 2019, 5:22 PM
evandro edited the summary of this revision. (Show Details)

Restrict the improvements to VS2015.

evandro marked 2 inline comments as done.Feb 5 2019, 5:23 PM
evandro edited the summary of this revision. (Show Details)Feb 5 2019, 5:25 PM
rnk added inline comments.Feb 5 2019, 5:28 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
168

I think it's common for non-clang users to not have any version info in the triple. I think in these cases, we are better off setting isPartialC99 to true. For example, most of the llc tests will use version-less triples. I was mainly interested in leaving a way for users to access the old behavior by setting an old MSVC version in the triple. Sorry for the extra work, I tried to spell it out in the comments.

evandro updated this revision to Diff 185462.Feb 5 2019, 6:33 PM
evandro edited the summary of this revision. (Show Details)

Assume VC19 as the default. However, I wonder if this should be the default earlier, in the clang driver.

evandro marked an inline comment as done.Feb 5 2019, 6:34 PM
mstorsjo added inline comments.Feb 6 2019, 12:11 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
182

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.
  1. 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).
225

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.

evandro marked 4 inline comments as done.Feb 6 2019, 9:41 AM
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
182

OK, I think that I understand what you mean. Stand by.

225

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.

evandro marked 3 inline comments as done.Feb 6 2019, 10:10 AM
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
182

Also, do you mean that that all C99 versions appeared in VC19?

evandro updated this revision to Diff 185601.Feb 6 2019, 11:30 AM

Separated functions between VC19 and float.

rnk accepted this revision.Feb 6 2019, 11:43 AM

I looked at the version checks and I'm happy with this. I haven't read all the discussion between you and @mstorsjo, but feel free to land if his concerns are addressed. Thanks for circling around and addressing this!

This revision is now accepted and ready to land.Feb 6 2019, 11:43 AM
mstorsjo added inline comments.Feb 6 2019, 12:55 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
225

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.

264

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.

evandro marked 4 inline comments as done.Feb 7 2019, 1:19 PM
evandro added a subscriber: dmajor.
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
225

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.

264

logbf was available in amd64 even before C99 was supported.

SP stands for single precision, AKA, float. I could rename it to hasPartialFloat.

mstorsjo added inline comments.Feb 8 2019, 1:24 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
225

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.

264

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");
evandro marked 4 inline comments as done.Feb 11 2019, 8:48 AM
evandro added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
264

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.

This revision was automatically updated to reflect the committed changes.
evandro marked an inline comment as done.

Commited extra float C99 functions in rGf4a369596f7b.

mstorsjo added inline comments.Feb 11 2019, 2:19 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
264

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.)

evandro marked an inline comment as done.Feb 11 2019, 2:21 PM