Bug 230437

Summary: Add LLVM/FLang's int128_t implementation to WTF
Product: WebKit Reporter: Philip Chimento <philip.chimento>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ap, benjamin, cdumez, cmarcelo, ews-watchlist, gyuyoung.kim, kevin_neal, ryuan.choi, sergio, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Description Philip Chimento 2021-09-17 17:33:37 PDT
Add LLVM/FLang's int128_t implementation to WTF
Comment 1 Philip Chimento 2021-09-17 17:34:35 PDT
This will be necessary for implementing Temporal.Instant.
Comment 2 Philip Chimento 2021-09-17 17:37:29 PDT
Created attachment 438537 [details]
Patch
Comment 3 Philip Chimento 2021-09-17 17:38:04 PDT
(The style checker is going to fail because the source files contain no copyright notices; I'm not sure what to do about that, because there are none in the original code.)
Comment 4 Yusuke Suzuki 2021-09-18 00:09:15 PDT
Comment on attachment 438537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438537&action=review

r=me with comments

> Source/WTF/wtf/CMakeLists.txt:106
> +    Int128.h

Add this header to xcodeproj.

> Source/WTF/wtf/CMakeLists.txt:117
> +    LeadingZeroBitCount.h

Add this header to xcodeproj.

> Source/WTF/wtf/LeadingZeroBitCount.h:41
> +static constexpr std::uint64_t deBruijn {0x07edd5e59a4e28c2};
> +static constexpr std::uint8_t mapping[64] {63, 0, 58, 1, 59, 47, 53, 2, 60, 39,
> +    48, 27, 54, 33, 42, 3, 61, 51, 37, 40, 49, 18, 28, 20, 55, 30, 34, 11, 43,
> +    14, 22, 4, 62, 57, 46, 52, 38, 26, 32, 41, 50, 36, 17, 19, 29, 10, 13, 21,
> +    56, 45, 25, 31, 35, 16, 9, 12, 44, 24, 15, 8, 23, 7, 6, 5};
> +} // namespace

You should create cpp file, and putting these constant arrays in C++ side. Otherwise, each translation unit will materialize this duplicate array, and enlarging binary size.

> Source/WTF/wtf/LeadingZeroBitCount.h:43
> +inline constexpr int LeadingZeroBitCount(std::uint64_t x)

Rename it to leadingZeroBitCount

> Source/WTF/wtf/LeadingZeroBitCount.h:64
> +inline constexpr int LeadingZeroBitCount(std::uint32_t x)

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:69
> +inline constexpr int LeadingZeroBitCount(std::uint16_t x)

Ditto

> Source/WTF/wtf/LeadingZeroBitCount.h:86
> +namespace detail {
> +static constexpr std::uint8_t eightBitLeadingZeroBitCount[256] {8, 7, 6, 6, 5, 5,
> +    5, 5, 4, 4, 4, 4, 4, 4, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
> +    3, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
> +    2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
> +    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
> +    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
> +}

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:88
> +inline constexpr int LeadingZeroBitCount(std::uint8_t x)

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:93
> +template <typename A> inline constexpr int BitsNeededFor(A x)

Rename it to bitsNeededFor.

> Tools/TestWebKitAPI/CMakeLists.txt:57
> +    Tests/WTF/Int128.cpp
>      Tests/WTF/IntegerToStringConversion.cpp
>      Tests/WTF/IteratorRange.cpp
>      Tests/WTF/JSONValue.cpp
>      Tests/WTF/LEBDecoder.cpp
> +    Tests/WTF/LeadingZeroBitCount.cpp

Add them to xcodeproj to run these tests on macOS / iOS too.
Comment 5 Alexey Proskuryakov 2021-09-19 12:47:37 PDT
If the license is not known, then we cannot accept this code in WebKit. I suspect that these projects do have licenses though.
Comment 6 Philip Chimento 2021-09-19 13:44:09 PDT
The license is known (and listed in the first lines of each source file), but the copyright is not.
Comment 7 Yusuke Suzuki 2021-09-19 16:01:13 PDT
(In reply to Alexey Proskuryakov from comment #5)
> If the license is not known, then we cannot accept this code in WebKit. I
> suspect that these projects do have licenses though.

It is LLVM, from where, we already have some files (B3ComputeDivisionMagic.h, old wtf/StdFilesystem.{cpp,h}, wtf/Span.h).
Let's just include https://llvm.org/LICENSE.txt file as WTF/LICENSE-LLVM.txt as https://trac.webkit.org/changeset/268020/webkit did.
Comment 8 Philip Chimento 2021-09-20 12:19:32 PDT
Created attachment 438710 [details]
Patch
Comment 9 Philip Chimento 2021-09-20 12:52:07 PDT
Created attachment 438712 [details]
Patch
Comment 10 Radar WebKit Bug Importer 2021-09-21 10:19:42 PDT
<rdar://problem/83357495>
Comment 11 EWS 2021-09-21 11:04:27 PDT
Committed r282828 (241959@main): <https://commits.webkit.org/241959@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438712 [details].