[WTF] Replace current LLVM flang's Int128 with abseil-cpp's Int128
Created attachment 442107 [details] Patch
Created attachment 442109 [details] Patch
Created attachment 442110 [details] Patch
Created attachment 442111 [details] Patch
Created attachment 442116 [details] Patch
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.
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.
Created attachment 442255 [details] Patch
Comment on attachment 442255 [details] Patch Thank you!
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].
<rdar://problem/84582748>