From df145e52286c28b75900a8cc23596339d0ed29d6 Mon Sep 17 00:00:00 2001 From: MikePopoloski Date: Sat, 9 Mar 2024 13:25:51 -0500 Subject: [PATCH] v1800-2023 clarification: time literals should be scaled but not rounded --- include/slang/numeric/Time.h | 2 +- source/ast/expressions/LiteralExpressions.cpp | 2 +- source/numeric/Time.cpp | 16 +++--- tests/unittests/ast/ExpressionTests.cpp | 55 ++++++++++++++++++- tests/unittests/util/NumericTests.cpp | 24 ++++---- 5 files changed, 77 insertions(+), 22 deletions(-) diff --git a/include/slang/numeric/Time.h b/include/slang/numeric/Time.h index 4eb5e6f24..3769865cd 100644 --- a/include/slang/numeric/Time.h +++ b/include/slang/numeric/Time.h @@ -63,7 +63,7 @@ struct SLANG_EXPORT TimeScale { TimeScale() = default; TimeScale(TimeScaleValue base, TimeScaleValue precision) : base(base), precision(precision) {} - double apply(double value, TimeUnit unit) const; + double apply(double value, TimeUnit unit, bool roundToPrecision) const; std::string toString() const; diff --git a/source/ast/expressions/LiteralExpressions.cpp b/source/ast/expressions/LiteralExpressions.cpp index 255bbf037..f7934ce69 100644 --- a/source/ast/expressions/LiteralExpressions.cpp +++ b/source/ast/expressions/LiteralExpressions.cpp @@ -139,7 +139,7 @@ Expression& TimeLiteral::fromSyntax(const ASTContext& context, double value = syntax.literal.realValue(); TimeUnit unit = syntax.literal.numericFlags().unit(); TimeScale scale = context.scope->getTimeScale().value_or(TimeScale()); - value = scale.apply(value, unit); + value = scale.apply(value, unit, /* roundToPrecision */ false); auto& comp = context.getCompilation(); return *comp.emplace(comp.getType(SyntaxKind::RealTimeType), value, diff --git a/source/numeric/Time.cpp b/source/numeric/Time.cpp index 95b891e4a..b3b4fcaa2 100644 --- a/source/numeric/Time.cpp +++ b/source/numeric/Time.cpp @@ -153,7 +153,7 @@ std::optional TimeScale::fromString(std::string_view str) { return TimeScale(*base, *precision); } -double TimeScale::apply(double value, TimeUnit unit) const { +double TimeScale::apply(double value, TimeUnit unit, bool roundToPrecision) const { // First scale the value by the difference between our base and the provided unit. // TimeUnits are from 0-5, so we need 11 entries. static constexpr double scales[] = {1e15, 1e12, 1e9, 1e6, 1e3, 1e0, @@ -162,12 +162,14 @@ double TimeScale::apply(double value, TimeUnit unit) const { double scale = scales[diff + int(TimeUnit::Femtoseconds)] / int(base.magnitude); value *= scale; - // Round the result to the number of decimals implied by the magnitude - // difference between our base and our precision. - diff = int(base.unit) - int(precision.unit); - scale = scales[diff + int(TimeUnit::Femtoseconds)] * int(base.magnitude); - scale /= int(precision.magnitude); - value = std::round(value * scale) / scale; + if (roundToPrecision) { + // Round the result to the number of decimals implied by the magnitude + // difference between our base and our precision. + diff = int(base.unit) - int(precision.unit); + scale = scales[diff + int(TimeUnit::Femtoseconds)] * int(base.magnitude); + scale /= int(precision.magnitude); + value = std::round(value * scale) / scale; + } return value; } diff --git a/tests/unittests/ast/ExpressionTests.cpp b/tests/unittests/ast/ExpressionTests.cpp index 09356bd42..53a476f20 100644 --- a/tests/unittests/ast/ExpressionTests.cpp +++ b/tests/unittests/ast/ExpressionTests.cpp @@ -1040,7 +1040,7 @@ endmodule NO_COMPILATION_ERRORS; auto& r = compilation.getRoot().lookupName("m.r"); - CHECK(r.getValue().real() == 234.057); + CHECK(r.getValue().real() == 234.0567891); } TEST_CASE("Fixed / dynamic array assignments") { @@ -3280,3 +3280,56 @@ endmodule CHECK(a.getValue().integer() == 0x700000000ull); CHECK(b.getValue().integer() == 4294967296ll); } + +TEST_CASE("Change of behavior in time literal rounding") { + // 1800-2023 changed the rules for time literals; previously they would + // be scaled and rounded to the current time settings, but now they are + // only scaled, not rounded. This is a breaking change, but it turns out + // they made this change because no other tools ever implemented the + // rounding part of the rule, so slang just needs to conform. + auto tree = SyntaxTree::fromText(R"x( +module test(); + timeunit 1ns/1ps; + + if (1.0 == 1ns) + $info("TEST1 PASSED"); + else + $info("TEST1 FAILED"); + + if (1fs == 10fs) + $info("TEST2 PASSED"); + else + $info("TEST2 FAILED"); + + if (1fs == 0.000001) + $info("TEST3 FAILED (SCALED/!ROUNDED)"); + else + if (1fs == 0.000000000000001) + $info("TEST3 FAILED (!SCALED/!ROUNDED)"); + else + if (1fs == 0.0) + $info("TEST3 PASSED"); + else + $info("TEST3 REALLY FAILED"); +endmodule : test +)x"); + + Compilation compilation; + compilation.addSyntaxTree(tree); + + // This used to print TEST PASSED for all three tests, + // but now it prints what all other tools print. + auto& diags = compilation.getAllDiagnostics(); + std::string result = "\n" + report(diags); + CHECK(result == R"x( +source:6:7: note: $info encountered: TEST1 PASSED + $info("TEST1 PASSED"); + ^ +source:13:7: note: $info encountered: TEST2 FAILED + $info("TEST2 FAILED"); + ^ +source:16:7: note: $info encountered: TEST3 FAILED (SCALED/!ROUNDED) + $info("TEST3 FAILED (SCALED/!ROUNDED)"); + ^ +)x"); +} diff --git a/tests/unittests/util/NumericTests.cpp b/tests/unittests/util/NumericTests.cpp index f758db179..a78c91d78 100644 --- a/tests/unittests/util/NumericTests.cpp +++ b/tests/unittests/util/NumericTests.cpp @@ -759,25 +759,25 @@ TEST_CASE("Time scaling") { auto ts = [](std::string_view str) { return TimeScale::fromString(str).value(); }; TimeScale scale = ts("100ns / 1ps"); - CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds) == AP(2.34057)); - CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds) == AP(0.00234)); - CHECK(scale.apply(234.0567891, TimeUnit::Seconds) == AP(2.340567891e9)); + CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds, true) == AP(2.34057)); + CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds, true) == AP(0.00234)); + CHECK(scale.apply(234.0567891, TimeUnit::Seconds, true) == AP(2.340567891e9)); scale.base = tv("10ps"); - CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds) == AP(23405.7)); - CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds) == AP(23.4)); - CHECK(scale.apply(234.0567891, TimeUnit::Seconds) == AP(2.340567891e13)); + CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds, true) == AP(23405.7)); + CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds, true) == AP(23.4)); + CHECK(scale.apply(234.0567891, TimeUnit::Seconds, true) == AP(2.340567891e13)); scale.base = tv("1ms"); - CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds) == AP(0.000234057)); - CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds) == AP(2.34e-7)); - CHECK(scale.apply(234.0567891, TimeUnit::Seconds) == AP(234056.7891)); + CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds, true) == AP(0.000234057)); + CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds, true) == AP(2.34e-7)); + CHECK(scale.apply(234.0567891, TimeUnit::Seconds, true) == AP(234056.7891)); scale.base = tv("1ns"); scale.precision = tv("1ns"); - CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds) == AP(234)); - CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds) == AP(0)); - CHECK(scale.apply(234.0567891, TimeUnit::Seconds) == AP(234056789100)); + CHECK(scale.apply(234.0567891, TimeUnit::Nanoseconds, true) == AP(234)); + CHECK(scale.apply(234.0567891, TimeUnit::Picoseconds, true) == AP(0)); + CHECK(scale.apply(234.0567891, TimeUnit::Seconds, true) == AP(234056789100)); } TEST_CASE("TimeScale stringify") {