[ImportVerilog][Moore] Add support for %t format specifier, introduce moore.fmt.time
#8979
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Scheremo
force-pushed
the
pr-t-format-specifier
branch
3 times, most recently
from
8f72551 to
7b45def
Compare
Scheremo
force-pushed
the
pr-t-format-specifier
branch
from
7b45def to
ae2423b
Compare
This PR adds support for using the %t format specifier in format strings by mapping them to the newly introduced moore.fmt.time / moore::TimeFormatOp in the ImportVerilog string formatter. According to the system verilog spec IEEE 1800-2023 § 20.4.3 "$timeformat", the exact formatting for any time value is dictated by the last $timeformat system task invoked within the scope or parent scopes of the format string, or alternatively, by a mix of constant default values and The smallest time precision argument of all the `timescale compiler directives in the source description. Because of the formatting relying on context (who came up with this anyway? seems like a bad idea), and use of $timeformat is permitted in conditional settings, meaning it can't always be determined statically, I decided to not add a format attribute to moore::FormatTimeOp. I only added the width attribute, which may be defined in the formatter expression, e.g. %4t. Instead the semantics for implementing moore.fmt.time should rely on querying some global state that models the configuration set by the latest $timeformat system call within the scope. What's not fully clear to me is whether the System Verilog spec allows non-"time typed" values to be formatted with %t; I didn't find an explicit description of allowed values, but all examples in the spec I could find explicitly use the return value of a time-typed system task (e.g. $time) or a time-type parameter (e.g. parameter time _time = 100ns;). While it seems reasonable to suppose that also integer values are allowed (per the spec $time returns a 64-bit integer representing time), some examples in the doc also allow the use of $realtime (which returns a real value) with %t, so potentially any numeric value is allowed? In either case I didn't add this in the current implementation and instead only allow TimeType values, which in the context of Moore looked like the sanest option to me. Maybe someone has a clearer view on this.
Scheremo
force-pushed
the
pr-t-format-specifier
branch
from
ae2423b to
5893656
Compare
Scheremo
changed the title
moore.fmt.timemoore.fmt.time
fabianschuiki
approved these changes
Sep 17, 2025
Contributor
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Very cool! I love the idea of keeping this simple and just work on TimeType. That will allow us to handle any $timeformat related business later on somewhere else.
Comment on lines
+218
to
+222
| // Only handle `TimeType` values. | ||
| auto value = context.convertRvalueExpression( | ||
| arg, moore::TimeType::get(context.getContext())); | ||
| if (!value) | ||
| return failure(); |
Contributor
There was a problem hiding this comment.
Nice! I think this will already handle integers properly, too. The conversion will insert casts from integers to the time type 🚀
fabianschuiki
approved these changes
Sep 17, 2025
Contributor
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Minor tweaks for easy commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for using the
%tformat specifier in format strings by mapping them to the newly introducedmoore.fmt.time / moore::TimeFormatOpin the ImportVerilog string formatter.According to the system verilog spec IEEE 1800-2023 § 20.4.3 "$timeformat", the exact formatting for any time value is dictated by the last
$timeformatsystem task invoked within the scope or parent scopes of the format string, or alternatively, by a mix of constant default values andBecause of the formatting relying on context (who came up with this anyway? seems like a bad idea), and use of
$timeformatis permitted in conditional settings, meaning it can't always be determined statically, I decided to not add a format attribute tomoore::FormatTimeOp. I only added thewidthattribute, which may be defined in the formatter expression, e.g.%4t. Instead the semantics for implementingmoore.fmt.timeshould rely on querying some global state that models the configuration set by the latest$timeformatsystem call within the scope.What's not fully clear to me is whether the System Verilog spec allows non-"time typed" values to be formatted with
%t; I didn't find an explicit description of allowed values, but all examples in the spec I could find explicitly use the return value of a time-typed system task (e.g.$time) or a time-type parameter (e.g.parameter time _time = 100ns;).While it seems reasonable to suppose that also integer values are allowed (per the spec
$timereturns a 64-bit integer representing time), some examples in the doc also allow the use of$realtime(which returns arealvalue) with%t, so potentially any numeric value is allowed? In either case I didn't add this in the current implementation and instead only allowTimeTypevalues, which in the context of Moore looked like the sanest option to me. Maybe someone has a clearer view on this.