Bug 232129 – [WTF] Replace current LLVM flang's Int128 with abseil-cpp's Int128

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[WTF] Replace current LLVM flang's Int128 with abseil-cpp's Int128

[Yusuke Suzuki]

Created attachment 442107 [details]
Patch

[Yusuke Suzuki]

Created attachment 442109 [details]
Patch

[Yusuke Suzuki]

Created attachment 442110 [details]
Patch

[Yusuke Suzuki]

Created attachment 442111 [details]
Patch

[Yusuke Suzuki]

Created attachment 442116 [details]
Patch

[Philip Chimento]

Comment on attachment 442116 [details]
Patch

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

Thanks for doing this so quickly! This int128 implementation looks more comprehensive than the Flang one anyway.

I would suggest removing WTF/LeadingZeroBitCount.{h,cpp} as well, because this implementation uses the clz() from WTF/MathExtras.h (which I didn't realize existed!) for counting leading zeros, and WTF::LeadingZeroBitCount was only added because the Flang int128 pulled it in.

(I don't have reviewer privileges, so I can't mark this r+, but otherwise LGTM)

> Source/WTF/wtf/Int128.h:81
> +// Additionally, if your compiler supports `__int128`, `UInt128Impl` is

If this functionality was removed when copying from Abseil, it's probably good to remove the comment as well, to avoid confusion.

> Source/WTF/wtf/Int128.h:100
> +//     float y = absl::UInt128Max();  // Error. UInt128Impl cannot be implicitly

Probably good to remove `absl::` here and in line 103 as well

> Source/WTF/wtf/Int128.h:292
> +// Additionally, if your compiler supports `__int128`, `Int128Impl` is

Same as earlier comment.

> Source/WTF/wtf/Int128.h:830
> +#else   // ABSL_HAVE_INTRINSIC128

This is a bit confusing. Although looking in the original source I see the code is actually correct when you remove the ABSL_HAVE_INTRINSIC128 case, because the MSVC&&x86_64 case was originally an #elif. So no need to change, but maybe consider just removing the comment.

> Tools/TestWebKitAPI/Tests/WTF/Int128.cpp:-79
> -static void TestVsNative(__uint128_t x, __uint128_t y)

I would suggest keeping the VsNative tests, and actually adding similar ones for the signed int128. These are probably the best indicator if something is wrong in the implementation. That's what originally alerted me to the Flang implementation's signed int128 operator>> not extending the sign.

[Yusuke Suzuki]

Comment on attachment 442116 [details]
Patch

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

>> Source/WTF/wtf/Int128.h:81
>> +// Additionally, if your compiler supports `__int128`, `UInt128Impl` is
> 
> If this functionality was removed when copying from Abseil, it's probably good to remove the comment as well, to avoid confusion.

Done.

>> Source/WTF/wtf/Int128.h:100
>> +//     float y = absl::UInt128Max();  // Error. UInt128Impl cannot be implicitly
> 
> Probably good to remove `absl::` here and in line 103 as well

Done.

>> Source/WTF/wtf/Int128.h:292
>> +// Additionally, if your compiler supports `__int128`, `Int128Impl` is
> 
> Same as earlier comment.

Done.

>> Source/WTF/wtf/Int128.h:830
>> +#else   // ABSL_HAVE_INTRINSIC128
> 
> This is a bit confusing. Although looking in the original source I see the code is actually correct when you remove the ABSL_HAVE_INTRINSIC128 case, because the MSVC&&x86_64 case was originally an #elif. So no need to change, but maybe consider just removing the comment.

Removed.

>> Tools/TestWebKitAPI/Tests/WTF/Int128.cpp:-79
>> -static void TestVsNative(__uint128_t x, __uint128_t y)
> 
> I would suggest keeping the VsNative tests, and actually adding similar ones for the signed int128. These are probably the best indicator if something is wrong in the implementation. That's what originally alerted me to the Flang implementation's signed int128 operator>> not extending the sign.

Done.

[Yusuke Suzuki]

Created attachment 442255 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 442255 [details]
Patch

Thank you!

[EWS]

Committed r284748 (243457@main): <https://commits.webkit.org/243457@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84582748>