From b6903295a826f5378b0445c36da7859373645e45 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Fri, 4 Nov 2022 19:18:55 +0000 Subject: [PATCH] tint/resolver: Support error cases with const-eval builtin tests Also: Print unrepresentable numbers with higher precision - otherwise values can round, and diagnostics can be very confusing. Improve diagnostic distinction between `( )` `[ ]` interval ranges. Change-Id: I9269fbf1738f0bce5f2ddb5a387687543fd5d0bb Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/108700 Commit-Queue: Antonio Maiorano Kokoro: Kokoro Reviewed-by: Antonio Maiorano --- src/tint/resolver/const_eval.cc | 28 +- .../resolver/const_eval_binary_op_test.cc | 4 +- src/tint/resolver/const_eval_builtin_test.cc | 288 +++++++++--------- 3 files changed, 159 insertions(+), 161 deletions(-) diff --git a/src/tint/resolver/const_eval.cc b/src/tint/resolver/const_eval.cc index 2554dc8a90..afb9f5dd1f 100644 --- a/src/tint/resolver/const_eval.cc +++ b/src/tint/resolver/const_eval.cc @@ -15,6 +15,7 @@ #include "src/tint/resolver/const_eval.h" #include +#include #include #include #include @@ -187,14 +188,24 @@ inline bool IsPositiveZero(T value) { template std::string OverflowErrorMessage(NumberT lhs, const char* op, NumberT rhs) { std::stringstream ss; + ss << std::setprecision(20); ss << "'" << lhs.value << " " << op << " " << rhs.value << "' cannot be represented as '" << FriendlyName() << "'"; return ss.str(); } +template +std::string OverflowErrorMessage(VALUE_TY value, std::string_view target_ty) { + std::stringstream ss; + ss << std::setprecision(20); + ss << "value " << value << " cannot be represented as " + << "'" << target_ty << "'"; + return ss.str(); +} + /// @returns the number of consecutive leading bits in `@p e` set to `@p bit_value_to_count`. template -auto CountLeadingBits(T e, T bit_value_to_count) -> std::make_unsigned_t { +std::make_unsigned_t CountLeadingBits(T e, T bit_value_to_count) { using UT = std::make_unsigned_t; constexpr UT kNumBits = sizeof(UT) * 8; constexpr UT kLeftMost = UT{1} << (kNumBits - 1); @@ -211,7 +222,7 @@ auto CountLeadingBits(T e, T bit_value_to_count) -> std::make_unsigned_t { /// @returns the number of consecutive trailing bits set to `@p bit_value_to_count` in `@p e` template -auto CountTrailingBits(T e, T bit_value_to_count) -> std::make_unsigned_t { +std::make_unsigned_t CountTrailingBits(T e, T bit_value_to_count) { using UT = std::make_unsigned_t; constexpr UT kNumBits = sizeof(UT) * 8; constexpr UT kRightMost = UT{1}; @@ -292,10 +303,9 @@ struct Element : ImplConstant { // --- Below this point are the failure cases --- } else if constexpr (IsAbstract) { // [abstract-numeric -> x] - materialization failure - std::stringstream ss; - ss << "value " << value << " cannot be represented as "; - ss << "'" << builder.FriendlyName(target_ty) << "'"; - builder.Diagnostics().add_error(tint::diag::System::Resolver, ss.str(), source); + builder.Diagnostics().add_error( + tint::diag::System::Resolver, + OverflowErrorMessage(value, builder.FriendlyName(target_ty)), source); return utils::Failure; } else if constexpr (IsFloatingPoint) { // [x -> floating-point] - number not exactly representable @@ -1602,7 +1612,7 @@ ConstEval::Result ConstEval::acos(const sem::Type* ty, auto create = [&](auto i) -> ImplResult { using NumberT = decltype(i); if (i < NumberT(-1.0) || i > NumberT(1.0)) { - AddError("acos must be called with a value in the range [-1, 1]", source); + AddError("acos must be called with a value in the range [-1 .. 1] (inclusive)", source); return utils::Failure; } return CreateElement(builder, c0->Type(), NumberT(std::acos(i.value))); @@ -1631,7 +1641,7 @@ ConstEval::Result ConstEval::asin(const sem::Type* ty, auto create = [&](auto i) -> ImplResult { using NumberT = decltype(i); if (i < NumberT(-1.0) || i > NumberT(1.0)) { - AddError("asin must be called with a value in the range [-1, 1]", source); + AddError("asin must be called with a value in the range [-1 .. 1] (inclusive)", source); return utils::Failure; } return CreateElement(builder, c0->Type(), NumberT(std::asin(i.value))); @@ -1677,7 +1687,7 @@ ConstEval::Result ConstEval::atanh(const sem::Type* ty, auto create = [&](auto i) -> ImplResult { using NumberT = decltype(i); if (i <= NumberT(-1.0) || i >= NumberT(1.0)) { - AddError("atanh must be called with a value in the range (-1, 1)", source); + AddError("atanh must be called with a value in the range (-1 .. 1) (exclusive)", source); return utils::Failure; } return CreateElement(builder, c0->Type(), NumberT(std::atanh(i.value))); diff --git a/src/tint/resolver/const_eval_binary_op_test.cc b/src/tint/resolver/const_eval_binary_op_test.cc index 05a1453c93..8708aa8b2f 100644 --- a/src/tint/resolver/const_eval_binary_op_test.cc +++ b/src/tint/resolver/const_eval_binary_op_test.cc @@ -853,7 +853,7 @@ TEST_F(ResolverConstEvalTest, BinaryAbstractAddOverflow_AFloat) { GlobalConst("c", Add(Source{{1, 1}}, Expr(AFloat::Highest()), AFloat::Highest())); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "1:1 error: '1.79769e+308 + 1.79769e+308' cannot be represented as 'abstract-float'"); + "1:1 error: '1.7976931348623157081e+308 + 1.7976931348623157081e+308' cannot be represented as 'abstract-float'"); } TEST_F(ResolverConstEvalTest, BinaryAbstractAddUnderflow_AFloat) { @@ -861,7 +861,7 @@ TEST_F(ResolverConstEvalTest, BinaryAbstractAddUnderflow_AFloat) { EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - "1:1 error: '-1.79769e+308 + -1.79769e+308' cannot be represented as 'abstract-float'"); + "1:1 error: '-1.7976931348623157081e+308 + -1.7976931348623157081e+308' cannot be represented as 'abstract-float'"); } // Mixed AInt and AFloat args to test implicit conversion to AFloat diff --git a/src/tint/resolver/const_eval_builtin_test.cc b/src/tint/resolver/const_eval_builtin_test.cc index 74ac16f143..3dc395b532 100644 --- a/src/tint/resolver/const_eval_builtin_test.cc +++ b/src/tint/resolver/const_eval_builtin_test.cc @@ -14,6 +14,8 @@ #include "src/tint/resolver/const_eval_test.h" +#include "src/tint/utils/result.h" + using namespace tint::number_suffixes; // NOLINT namespace tint::resolver { @@ -23,25 +25,39 @@ namespace { using resolver::operator<<; struct Case { - Case(utils::VectorRef in_args, Types in_expected) - : args(std::move(in_args)), expected(std::move(in_expected)) {} + Case(utils::VectorRef in_args, Types expected_value) + : args(std::move(in_args)), expected(Success{std::move(expected_value), false, false}) {} + + Case(utils::VectorRef in_args, const char* expected_err) + : args(std::move(in_args)), expected(Failure{expected_err}) {} /// Expected value may be positive or negative Case& PosOrNeg() { - expected_pos_or_neg = true; + Success s = expected.Get(); + s.pos_or_neg = true; + expected = s; return *this; } /// Expected value should be compared using FLOAT_EQ instead of EQ Case& FloatComp() { - float_compare = true; + Success s = expected.Get(); + s.float_compare = true; + expected = s; return *this; } + struct Success { + Types value; + bool pos_or_neg = false; + bool float_compare = false; + }; + struct Failure { + const char* error = nullptr; + }; + utils::Vector args; - Types expected; - bool expected_pos_or_neg = false; - bool float_compare = false; + utils::Result expected; }; static std::ostream& operator<<(std::ostream& o, const Case& c) { @@ -49,17 +65,24 @@ static std::ostream& operator<<(std::ostream& o, const Case& c) { for (auto& a : c.args) { o << a << ", "; } - o << "expected: " << c.expected << ", expected_pos_or_neg: " << c.expected_pos_or_neg; + o << "expected: "; + if (c.expected) { + auto s = c.expected.Get(); + o << s.value << ", pos_or_neg: " << s.pos_or_neg; + } else { + o << "[ERROR: " << c.expected.Failure().error << "]"; + } return o; } +using ScalarTypes = std::variant; + /// Creates a Case with Values for args and result static Case C(std::initializer_list args, Types result) { return Case{utils::Vector{args}, std::move(result)}; } /// Convenience overload that creates a Case with just scalars -using ScalarTypes = std::variant; static Case C(std::initializer_list sargs, ScalarTypes sresult) { utils::Vector args; for (auto& sa : sargs) { @@ -70,6 +93,20 @@ static Case C(std::initializer_list sargs, ScalarTypes sresult) { return Case{std::move(args), std::move(result)}; } +/// Creates a Case with Values for args and expected error +static Case E(std::initializer_list args, const char* err) { + return Case{utils::Vector{args}, err}; +} + +/// Convenience overload that creates an expected-error Case with just scalars +static Case E(std::initializer_list sargs, const char* err) { + utils::Vector args; + for (auto& sa : sargs) { + std::visit([&](auto&& v) { return args.Push(Val(v)); }, sa); + } + return Case{std::move(args), err}; +} + using ResolverConstEvalBuiltinTest = ResolverTestWithParam>; TEST_P(ResolverConstEvalBuiltinTest, Test) { @@ -83,58 +120,65 @@ TEST_P(ResolverConstEvalBuiltinTest, Test) { std::visit([&](auto&& v) { args.Push(v.Expr(*this)); }, a); } - auto* expected = ToValueBase(c.expected); - auto* expr = Call(sem::str(builtin), std::move(args)); + auto* expr = Call(Source{{12, 34}}, sem::str(builtin), std::move(args)); GlobalConst("C", expr); - auto* expected_expr = expected->Expr(*this); - GlobalConst("E", expected_expr); - ASSERT_TRUE(r()->Resolve()) << r()->error(); + if (c.expected) { + auto expected = c.expected.Get(); - auto* sem = Sem().Get(expr); - ASSERT_NE(sem, nullptr); - const sem::Constant* value = sem->ConstantValue(); - ASSERT_NE(value, nullptr); - EXPECT_TYPE(value->Type(), sem->Type()); + auto* expected_expr = ToValueBase(expected.value)->Expr(*this); + GlobalConst("E", expected_expr); - auto* expected_sem = Sem().Get(expected_expr); - const sem::Constant* expected_value = expected_sem->ConstantValue(); - ASSERT_NE(expected_value, nullptr); - EXPECT_TYPE(expected_value->Type(), expected_sem->Type()); + ASSERT_TRUE(r()->Resolve()) << r()->error(); - // @TODO(amaiorano): Rewrite using ScalarArgsFrom() - ForEachElemPair(value, expected_value, [&](const sem::Constant* a, const sem::Constant* b) { - std::visit( - [&](auto&& ct_expected) { - using T = typename std::decay_t::ElementType; + auto* sem = Sem().Get(expr); + ASSERT_NE(sem, nullptr); + const sem::Constant* value = sem->ConstantValue(); + ASSERT_NE(value, nullptr); + EXPECT_TYPE(value->Type(), sem->Type()); - auto v = a->As(); - auto e = b->As(); - if constexpr (std::is_same_v) { - EXPECT_EQ(v, e); - } else if constexpr (IsFloatingPoint) { - if (std::isnan(e)) { - EXPECT_TRUE(std::isnan(v)); - } else { - auto vf = (c.expected_pos_or_neg ? Abs(v) : v); - if (c.float_compare) { - EXPECT_FLOAT_EQ(vf, e); + auto* expected_sem = Sem().Get(expected_expr); + const sem::Constant* expected_value = expected_sem->ConstantValue(); + ASSERT_NE(expected_value, nullptr); + EXPECT_TYPE(expected_value->Type(), expected_sem->Type()); + + // @TODO(amaiorano): Rewrite using ScalarArgsFrom() + ForEachElemPair(value, expected_value, [&](const sem::Constant* a, const sem::Constant* b) { + std::visit( + [&](auto&& ct_expected) { + using T = typename std::decay_t::ElementType; + + auto v = a->As(); + auto e = b->As(); + if constexpr (std::is_same_v) { + EXPECT_EQ(v, e); + } else if constexpr (IsFloatingPoint) { + if (std::isnan(e)) { + EXPECT_TRUE(std::isnan(v)); } else { - EXPECT_EQ(vf, e); + auto vf = (expected.pos_or_neg ? Abs(v) : v); + if (expected.float_compare) { + EXPECT_FLOAT_EQ(vf, e); + } else { + EXPECT_EQ(vf, e); + } } + } else { + EXPECT_EQ((expected.pos_or_neg ? Abs(v) : v), e); + // Check that the constant's integer doesn't contain unexpected + // data in the MSBs that are outside of the bit-width of T. + EXPECT_EQ(a->As(), b->As()); } - } else { - EXPECT_EQ((c.expected_pos_or_neg ? Abs(v) : v), e); - // Check that the constant's integer doesn't contain unexpected - // data in the MSBs that are outside of the bit-width of T. - EXPECT_EQ(a->As(), b->As()); - } - }, - c.expected); + }, + expected.value); - return HasFailure() ? Action::kStop : Action::kContinue; - }); + return HasFailure() ? Action::kStop : Action::kContinue; + }); + } else { + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), c.expected.Failure().error); + } } INSTANTIATE_TEST_SUITE_P( // @@ -374,6 +418,19 @@ std::vector AtanhCases() { C({Vec(T(0.0), T(0.9), -T(0.9))}, Vec(T(0.0), T(1.4722193), -T(1.4722193))).FloatComp(), }; + ConcatIntoIf( // + cases, + std::vector{ + E({1.1_a}, + "12:34 error: atanh must be called with a value in the range (-1 .. 1) (exclusive)"), + E({-1.1_a}, + "12:34 error: atanh must be called with a value in the range (-1 .. 1) (exclusive)"), + E({T::Inf()}, + "12:34 error: atanh must be called with a value in the range (-1 .. 1) (exclusive)"), + E({-T::Inf()}, + "12:34 error: atanh must be called with a value in the range (-1 .. 1) (exclusive)"), + }); + ConcatIntoIf( // cases, std::vector{ // If i is NaN, NaN is returned @@ -393,38 +450,6 @@ INSTANTIATE_TEST_SUITE_P( // AtanhCases(), AtanhCases())))); -TEST_F(ResolverConstEvalBuiltinTest, Atanh_OutsideRange_Positive) { - auto* expr = Call(Source{{12, 24}}, "atanh", Expr(1.0_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: atanh must be called with a value in the range (-1, 1)"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Atanh_OutsideRange_Negative) { - auto* expr = Call(Source{{12, 24}}, "atanh", Negation(1.0_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: atanh must be called with a value in the range (-1, 1)"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Atanh_OutsideRange_Positive_INF) { - auto* expr = Call(Source{{12, 24}}, "atanh", Expr(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: atanh must be called with a value in the range (-1, 1)"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Atanh_OutsideRange_Negative_INF) { - auto* expr = Call(Source{{12, 24}}, "atanh", Negation(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: atanh must be called with a value in the range (-1, 1)"); -} - template std::vector AcosCases() { std::vector cases = { @@ -438,6 +463,19 @@ std::vector AcosCases() { C({Vec(T(1.0), -T(1.0))}, Vec(T(0), kPi)).FloatComp(), }; + ConcatIntoIf( // + cases, + std::vector{ + E({1.1_a}, + "12:34 error: acos must be called with a value in the range [-1 .. 1] (inclusive)"), + E({-1.1_a}, + "12:34 error: acos must be called with a value in the range [-1 .. 1] (inclusive)"), + E({T::Inf()}, + "12:34 error: acos must be called with a value in the range [-1 .. 1] (inclusive)"), + E({-T::Inf()}, + "12:34 error: acos must be called with a value in the range [-1 .. 1] (inclusive)"), + }); + ConcatIntoIf( // cases, std::vector{ // If i is NaN, NaN is returned @@ -457,38 +495,6 @@ INSTANTIATE_TEST_SUITE_P( // AcosCases(), AcosCases())))); -TEST_F(ResolverConstEvalBuiltinTest, Acos_OutsideRange_Positive) { - auto* expr = Call(Source{{12, 24}}, "acos", Expr(1.1_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: acos must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Acos_OutsideRange_Negative) { - auto* expr = Call(Source{{12, 24}}, "acos", Negation(1.1_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: acos must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Acos_OutsideRange_Positive_INF) { - auto* expr = Call(Source{{12, 24}}, "acos", Expr(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: acos must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Acos_OutsideRange_Negative_INF) { - auto* expr = Call(Source{{12, 24}}, "acos", Negation(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: acos must be called with a value in the range [-1, 1]"); -} - template std::vector AsinCases() { std::vector cases = { @@ -503,6 +509,19 @@ std::vector AsinCases() { C({Vec(T(0.0), T(1.0), -T(1.0))}, Vec(T(0.0), kPiOver2, -kPiOver2)).FloatComp(), }; + ConcatIntoIf( // + cases, + std::vector{ + E({1.1_a}, + "12:34 error: asin must be called with a value in the range [-1 .. 1] (inclusive)"), + E({-1.1_a}, + "12:34 error: asin must be called with a value in the range [-1 .. 1] (inclusive)"), + E({T::Inf()}, + "12:34 error: asin must be called with a value in the range [-1 .. 1] (inclusive)"), + E({-T::Inf()}, + "12:34 error: asin must be called with a value in the range [-1 .. 1] (inclusive)"), + }); + ConcatIntoIf( // cases, std::vector{ // If i is NaN, NaN is returned @@ -522,38 +541,6 @@ INSTANTIATE_TEST_SUITE_P( // AsinCases(), AsinCases())))); -TEST_F(ResolverConstEvalBuiltinTest, Asin_OutsideRange_Positive) { - auto* expr = Call(Source{{12, 24}}, "asin", Expr(1.1_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: asin must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Asin_OutsideRange_Negative) { - auto* expr = Call(Source{{12, 24}}, "asin", Negation(1.1_a)); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: asin must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Asin_OutsideRange_Positive_INF) { - auto* expr = Call(Source{{12, 24}}, "asin", Expr(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: asin must be called with a value in the range [-1, 1]"); -} - -TEST_F(ResolverConstEvalBuiltinTest, Asin_OutsideRange_Negative_INF) { - auto* expr = Call(Source{{12, 24}}, "asin", Negation(f32::Inf())); - - GlobalConst("C", expr); - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:24 error: asin must be called with a value in the range [-1, 1]"); -} - template std::vector AsinhCases() { std::vector cases = { @@ -980,12 +967,12 @@ using ResolverConstEvalBuiltinTest_InsertBits_InvalidOffsetAndCount = ResolverTestWithParam>; TEST_P(ResolverConstEvalBuiltinTest_InsertBits_InvalidOffsetAndCount, Test) { auto& p = GetParam(); - auto* expr = Call(Source{{12, 24}}, sem::str(sem::BuiltinType::kInsertBits), Expr(1_u), + auto* expr = Call(Source{{12, 34}}, sem::str(sem::BuiltinType::kInsertBits), Expr(1_u), Expr(1_u), Expr(u32(std::get<0>(p))), Expr(u32(std::get<1>(p)))); GlobalConst("C", expr); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:24 error: 'offset + 'count' must be less than or equal to the bit width of 'e'"); + "12:34 error: 'offset + 'count' must be less than or equal to the bit width of 'e'"); } INSTANTIATE_TEST_SUITE_P(InsertBits, ResolverConstEvalBuiltinTest_InsertBits_InvalidOffsetAndCount, @@ -1083,12 +1070,12 @@ using ResolverConstEvalBuiltinTest_ExtractBits_InvalidOffsetAndCount = ResolverTestWithParam>; TEST_P(ResolverConstEvalBuiltinTest_ExtractBits_InvalidOffsetAndCount, Test) { auto& p = GetParam(); - auto* expr = Call(Source{{12, 24}}, sem::str(sem::BuiltinType::kExtractBits), Expr(1_u), + auto* expr = Call(Source{{12, 34}}, sem::str(sem::BuiltinType::kExtractBits), Expr(1_u), Expr(u32(std::get<0>(p))), Expr(u32(std::get<1>(p)))); GlobalConst("C", expr); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:24 error: 'offset + 'count' must be less than or equal to the bit width of 'e'"); + "12:34 error: 'offset + 'count' must be less than or equal to the bit width of 'e'"); } INSTANTIATE_TEST_SUITE_P(ExtractBits, ResolverConstEvalBuiltinTest_ExtractBits_InvalidOffsetAndCount, @@ -1282,6 +1269,7 @@ INSTANTIATE_TEST_SUITE_P( // StepCases())))); std::vector QuantizeToF16Cases() { + (void)E({Vec(0_f, 0_f)}, ""); // Currently unused, but will be soon. return { C({0_f}, 0_f), // C({-0_f}, -0_f), //