Skip to content

Commit

Permalink
v1800-2023 clarification: time literals should be scaled but not rounded
Browse files Browse the repository at this point in the history
  • Loading branch information
@MikePopoloski
MikePopoloski committed Mar 9, 2024
1 parent 57db172 commit df145e5
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 22 deletions.
2 changes: 1 addition & 1 deletion include/slang/numeric/Time.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion source/ast/expressions/LiteralExpressions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<TimeLiteral>(comp.getType(SyntaxKind::RealTimeType), value,
Expand Down
16 changes: 9 additions & 7 deletions source/numeric/Time.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ std::optional<TimeScale> 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,
Expand All @@ -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;
}
Expand Down
55 changes: 54 additions & 1 deletion tests/unittests/ast/ExpressionTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,7 @@ endmodule
NO_COMPILATION_ERRORS;

auto& r = compilation.getRoot().lookupName<ParameterSymbol>("m.r");
CHECK(r.getValue().real() == 234.057);
CHECK(r.getValue().real() == 234.0567891);
}

TEST_CASE("Fixed / dynamic array assignments") {
Expand Down Expand Up @@ -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");
}
24 changes: 12 additions & 12 deletions tests/unittests/util/NumericTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down

0 comments on commit df145e5

Please sign in to comment.