-
Notifications
You must be signed in to change notification settings - Fork 404
[Moore] Add proper conversions between time and integer values #8900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for conversion operation stuff except for one nit about naming of time_to_int. I think canonicalization and FVInt changes are orthogonal to conversion changes and I have a few question regarding behavior of div-by-zero.
| assert(exp <= 5); | ||
| exp = 5 - exp; | ||
| auto scale = static_cast<uint64_t>(context.timeScale.base.magnitude); | ||
| while (exp-- > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL there is no integer version of std::pow 😄 Does slang warn if there is time literal that is out-of-range (and causes overflow here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 😅. I think this here is always safe, because scale is 1, 10, or 100, and exp is between 0 and 5. So this produces 1000^5 in the worst case, which fits into 52 bits.
| }]; | ||
| } | ||
|
|
||
| def TimeToIntOp : MooreOp<"time_to_int", [Pure]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maye time_to_logic is better name? I thought it must produce a two state value because of _int suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it four valued here? (e.g., I'd have assumed time would be two state valued)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| value.getContext(), dstInt.getWidth(), srcPacked.getDomain()); | ||
| if (dstInt.getWidth() < srcInt.getWidth()) { | ||
| value = moore::TruncOp::create(builder, loc, resizedType, value); | ||
| value = builder.createOrFold<moore::TruncOp>(loc, resizedType, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate that TruncOp::createOrFold doesn't exist yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this does change the uniformity a bit ... I'll see about adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be very neat 💯
include/circt/Support/FVInt.h
Outdated
| return v; | ||
| } | ||
|
|
||
| /// Compute an unsigned division. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have references for these semantics? Should div-by-zero return X value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a reference to the SV spec. Good point about the div-by-zero producing X. I forgot about that. Added a test for it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!
| }]; | ||
| } | ||
|
|
||
| def TimeToIntOp : MooreOp<"time_to_int", [Pure]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it four valued here? (e.g., I'd have assumed time would be two state valued)
| value.getContext(), dstInt.getWidth(), srcPacked.getDomain()); | ||
| if (dstInt.getWidth() < srcInt.getWidth()) { | ||
| value = moore::TruncOp::create(builder, loc, resizedType, value); | ||
| value = builder.createOrFold<moore::TruncOp>(loc, resizedType, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this does change the uniformity a bit ... I'll see about adding.
| let hasFolder = 1; | ||
| } | ||
|
|
||
| def IntToTimeOp : MooreOp<"int_to_time", [Pure]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can either of these either fail or are all inputs valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All inputs are valid. Sadly, time values are just unit-less integers in Verilog, so any conversion issues or unit mismatches are basically mandated by the standard 🤦♂️
Time values in Verilog are basically just plain 64 bit integers. This is an infinite source of frustration when trying to send time values between units with different timescales. The Moore dialect has a separate `!moore.time` type for this reason, allowing time values to be captured more precisely and converted to the equivalent `!llhd.time`. To allow users to write `#5`, basically delaying execution for 5 times the local time scale value, add a `moore.time_to_int` and complementary `moore.int_to_time` operation. These allow time values to be cast from and to their equivalent integer number of femtoseconds. Make type conversions in ImportVerilog more conservative by refusing any conversion between packed and simple bit vector types if the packed type has any nested time value. For trivial casts between an integer and a time value, use the new `moore.time_to_int` and `moore.int_to_time` operations together with a `moore.mul` or `moore.divu` of the integer value by the local timescale value. Add unsigned and signed division operators to `FVInt`. Add a bunch of constant folders to the Moore dialect and switch over to `createOrFold` in most conversion functions to produce already-folded IR where possible. This now allows circt-verilog to process statements like `#5` or `time t = 42`.
3b63902 to
f38c6fa
Compare

Time values in Verilog are basically just plain 64 bit integers. This is an infinite source of frustration when trying to send time values between units with different timescales. The Moore dialect has a separate
!moore.timetype for this reason, allowing time values to be captured more precisely and converted to the equivalent!llhd.time.To allow users to write
#5, basically delaying execution for 5 times the local time scale value, add amoore.time_to_intand complementarymoore.int_to_timeoperation. These allow time values to be cast from and to their equivalent integer number of femtoseconds.Make type conversions in ImportVerilog more conservative by refusing any conversion between packed and simple bit vector types if the packed type has any nested time value. For trivial casts between an integer and a time value, use the new
moore.time_to_intandmoore.int_to_timeoperations together with amoore.mulormoore.divuof the integer value by the local timescale value.Add unsigned and signed division operators to
FVInt.Add a bunch of constant folders to the Moore dialect and switch over to
createOrFoldin most conversion functions to produce already-folded IR where possible.This now allows circt-verilog to process statements like
#5ortime t = 42.