From 2b4df7889186d2e98e437a2d1bb3e083b4722edb Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Wed, 29 Jun 2022 11:31:41 +0000
Subject: [PATCH] tint/number: Fix CheckedConvert() logic

This function was attempting to pick a higher-precision type by using
the decltype() of FROM + TO. This doesn't work if FROM and TO are both
similar bit-widths, and of a different signness, as the picked type may
not be wide enough to hold both the signed and unsigned representation.

Just use AInt or AFloat (both 64-bit), which are the largest types
supported by WGSL.

Change-Id: Ic76475d98bad8def12a0283a1c83c62f2ed58b5d
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/95041
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@google.com>
---
 src/tint/number.h       |  4 +++-
 src/tint/number_test.cc | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/tint/number.h b/src/tint/number.h
index 7f0c2f1005..4e3e6fd317 100644
--- a/src/tint/number.h
+++ b/src/tint/number.h
@@ -282,7 +282,9 @@ std::ostream& operator<<(std::ostream& out, ConversionFailure failure);
 /// @returns the resulting value of the conversion, or a failure reason.
 template <typename TO, typename FROM>
 utils::Result<TO, ConversionFailure> CheckedConvert(Number<FROM> num) {
-    using T = decltype(UnwrapNumber<TO>() + num.value);
+    // Use the highest-precision integer or floating-point type to perform the comparisons.
+    using T = std::conditional_t<IsFloatingPoint<UnwrapNumber<TO>> || IsFloatingPoint<FROM>,
+                                 AFloat::type, AInt::type>;
     const auto value = static_cast<T>(num.value);
     if (value > static_cast<T>(TO::kHighest)) {
         return ConversionFailure::kExceedsPositiveLimit;
diff --git a/src/tint/number_test.cc b/src/tint/number_test.cc
index 52ba4aea3e..6a48f9d7eb 100644
--- a/src/tint/number_test.cc
+++ b/src/tint/number_test.cc
@@ -84,6 +84,7 @@ TEST(NumberTest, CheckedConvertIdentity) {
 TEST(NumberTest, CheckedConvertLargestValue) {
     EXPECT_EQ(CheckedConvert<i32>(AInt(kHighestI32)), i32(kHighestI32));
     EXPECT_EQ(CheckedConvert<u32>(AInt(kHighestU32)), u32(kHighestU32));
+    EXPECT_EQ(CheckedConvert<u32>(i32(kHighestI32)), u32(kHighestI32));
     EXPECT_EQ(CheckedConvert<f32>(AFloat(kHighestF32)), f32(kHighestF32));
     EXPECT_EQ(CheckedConvert<f16>(AFloat(kHighestF16)), f16(kHighestF16));
 }
@@ -105,6 +106,14 @@ TEST(NumberTest, CheckedConvertSmallestValue) {
 TEST(NumberTest, CheckedConvertExceedsPositiveLimit) {
     EXPECT_EQ(CheckedConvert<i32>(AInt(kHighestI32 + 1)), ConversionFailure::kExceedsPositiveLimit);
     EXPECT_EQ(CheckedConvert<u32>(AInt(kHighestU32 + 1)), ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<i32>(u32(kHighestU32)), ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<i32>(u32(0x80000000)), ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<u32>(f32(f32::kHighest)), ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<i32>(f32(f32::kHighest)), ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<u32>(AFloat(AFloat::kHighest)),
+              ConversionFailure::kExceedsPositiveLimit);
+    EXPECT_EQ(CheckedConvert<i32>(AFloat(AFloat::kHighest)),
+              ConversionFailure::kExceedsPositiveLimit);
     EXPECT_EQ(CheckedConvert<f32>(AFloat(kHighestF32NextULP)),
               ConversionFailure::kExceedsPositiveLimit);
     EXPECT_EQ(CheckedConvert<f16>(AFloat(kHighestF16NextULP)),
@@ -114,6 +123,14 @@ TEST(NumberTest, CheckedConvertExceedsPositiveLimit) {
 TEST(NumberTest, CheckedConvertExceedsNegativeLimit) {
     EXPECT_EQ(CheckedConvert<i32>(AInt(kLowestI32 - 1)), ConversionFailure::kExceedsNegativeLimit);
     EXPECT_EQ(CheckedConvert<u32>(AInt(kLowestU32 - 1)), ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<u32>(i32(-1)), ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<u32>(i32(kLowestI32)), ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<u32>(f32(f32::kLowest)), ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<i32>(f32(f32::kLowest)), ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<u32>(AFloat(AFloat::kLowest)),
+              ConversionFailure::kExceedsNegativeLimit);
+    EXPECT_EQ(CheckedConvert<i32>(AFloat(AFloat::kLowest)),
+              ConversionFailure::kExceedsNegativeLimit);
     EXPECT_EQ(CheckedConvert<f32>(AFloat(kLowestF32NextULP)),
               ConversionFailure::kExceedsNegativeLimit);
     EXPECT_EQ(CheckedConvert<f16>(AFloat(kLowestF16NextULP)),