This is an archive of the discontinued LLVM Phabricator instance.

[7/N] [libcxx] Don't use __int128 for msvc targets
AbandonedPublic

Authored by mstorsjo on Nov 10 2020, 1:36 AM.

Details

Reviewers
rnk
ldionne
EricWF
amccarth
Group Reviewers
Restricted Project
Summary

While clang-cl can compile it just fine, the compiler support lib lacks support for it (as compiler-rt builtins aren't used).

This means that file_time_type has a shorter range on this target (+/- 292 years from the epoch). This is smaller than the range of the windows native file time type (which is 64 bit, with a 100 ns unit, from an epoch of year 1601), but covers the practically used range just fine. See docs/DesignDocs/FileTimeType.rst for more of the tradeoffs this means.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 1:36 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Nov 10 2020, 1:36 AM
ldionne requested changes to this revision.Nov 10 2020, 8:41 AM
ldionne added a subscriber: ldionne.

This influences the ABI IIRC, so this choice may be forever. Shouldn't we fix the compiler support lib on Windows instead?

This revision now requires changes to proceed.Nov 10 2020, 8:41 AM

This influences the ABI IIRC, so this choice may be forever.

Well afaik the ABI of libc++ on windows is rather non-fixed so far anyway

Shouldn't we fix the compiler support lib on Windows instead?

The thing is, in setups when building with clang-cl, i.e. clang operating in MSVC mode, we only use the MSVC provided support libs, so there's not much we can fix ourselves there.

@rnk, @thakis, what do you guys think?

I guess the alternative is to lift implementations of __divti3 and __udivti3 from compiler-rt and place in libcxx/src/filesystem - there's already a copy of __muloti4 there, due to libgcc missing it, and we could provide those other two as well, for msvc builds.

I guess the alternative is to lift implementations of __divti3 and __udivti3 from compiler-rt and place in libcxx/src/filesystem - there's already a copy of __muloti4 there, due to libgcc missing it, and we could provide those other two as well, for msvc builds.

Made such an alternative fix in D91413.

amccarth resigned from this revision.Dec 2 2020, 3:10 PM

I'm out of my depth on 128-bit arithmetic and ABIs.

rnk added a comment.Dec 2 2020, 3:13 PM

After having dealt with an increasing number of user complaints about lack of i128 support, I am of the opinion that we should start shipping, auto-linking, and requiring clang_rt.builtins in clang-cl.

However, it's a bit of a project to make this work out of the box, since clang's runtime libraries are not on the linker search path. We'd need to update our msbuild integration bits and some documentation. But it's time to do that work already.

In D91139#2429595, @rnk wrote:

After having dealt with an increasing number of user complaints about lack of i128 support, I am of the opinion that we should start shipping, auto-linking, and requiring clang_rt.builtins in clang-cl.

However, it's a bit of a project to make this work out of the box, since clang's runtime libraries are not on the linker search path. We'd need to update our msbuild integration bits and some documentation. But it's time to do that work already.

So, if I understand correctly, you're suggesting that the int128 builtins should be provided on Windows, and this workaround shouldn't be necessary once that's done? In that case, unless @mstorsjo disagrees, I think we're better holding off with this patch.

In D91139#2429595, @rnk wrote:

After having dealt with an increasing number of user complaints about lack of i128 support, I am of the opinion that we should start shipping, auto-linking, and requiring clang_rt.builtins in clang-cl.

However, it's a bit of a project to make this work out of the box, since clang's runtime libraries are not on the linker search path. We'd need to update our msbuild integration bits and some documentation. But it's time to do that work already.

So, if I understand correctly, you're suggesting that the int128 builtins should be provided on Windows, and this workaround shouldn't be necessary once that's done? In that case, unless @mstorsjo disagrees, I think we're better holding off with this patch.

Yes, I'm fine holding off this patch for now - it's not needed for the rest of the series on mingw targets (that do use either libgcc or compiler-rt builtins) anyway.

mstorsjo abandoned this revision.Dec 6 2020, 1:52 PM

Abandoning this one for now, see https://reviews.llvm.org/D91139#2429595 for the supposed path forward for MSVC configurations. For mingw configurations, this patch isn't needed and everything else needed for std::filesystem is available.