-
Notifications
You must be signed in to change notification settings - Fork 417
[MooreToCore][Sim] Added support for moore.fmt.real
#9397
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
|
Thank you for working on the formatting ops 👍 It is also not obvious what should be its own operation and what can be controlled by an attribute, see #7692. I think my current preference (at least for the |
e305970 to
7ec95b3
Compare
7ec95b3 to
6e7d18b
Compare
6e7d18b to
e305970
Compare
e305970 to
5af79c2
Compare
|
@darthscsi @seldridge, sorry to add you as reviewers - something had gone wrong with a prior rebase |
Ah, I see - I think that the patch (both int and real formatting) might be helpful and serve as reference code for future library implementations (as discussed with @fabianschuiki), so it might be helpful to keep it in. What do you think? |
|
@fabianschuiki, could you please take a final look and merge? |
fabianschuiki
left a comment
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! Thanks a lot for plumbing this all the way through to the Sim dialect, adding thorough tests, and including a constant folding implementation as a reference. Very cool stuff! Glad to see that you're getting rid of 46 errors in sv-tests 😏 💯 🥳
Since LLVM doesn't have a very clean separation between printing out floats in FP vs Scientific Notation (see
toStringImplhere), I decided to do what Verilator does here : callsnprintf. So for now, we're doing C-style prints - I plan to make a patch to LLVM to disambiguatetoStringImplfor FP notation, but that's for later.Overview:
%fand%e$fieldWidthis used to determine padding, and$fracDigitsis used to denote the number of digits after the decimal point/the alphabet 'e' - the actual formatting is handled bysnprintfunder the hood%ghas a unique formatting logic:For
exp ≥ 0:Uses decimal notation if
exp + 1 ≤ max(1, fracDigits), otherwise switches to scientific notation withmax(0, fracDigits - 1)fractional digitsFor
exp < 0:If
exp ≥ -4: Uses decimal notation withmax(b - 1, 0) + |exp|fractional digitsIf
exp < -4: Uses scientific notation withmax(b - 1, 0)fractional digitsEverything is stripped of trailing zeroes after the decimal point/the alphabet 'e'
which, as expected, is in conjunction with this
I've introduced this patch to fix the current
fmt.reallegalization errors and improve test coverage. I think it is beneficial to merge this even if we remove the formatting folders later, since this might serve as reference code for the arcilator runtime (Relevant Issue)Error diff (against current
main):