Skip to content
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

core: Print fp literals losslessly #3381

Merged
merged 13 commits into from
Nov 4, 2024
Merged

Conversation

n-io
Copy link
Collaborator

@n-io n-io commented Nov 1, 2024

mlir-opt attempts to print floats as 6dp scientific notation, but round-trips to ensure there is no precision loss and the printed number will bitwise reproduce exactly. If this fails, it will choose a different printing method to ensure bit-reproducible printing for a given precision of fp types.

This PR introduces a flag to print with full precision. This should eventually become the default.
This PR prints with full precision by default, but provides a --print-reduced-precision-fp flag to enable the legacy behaviour.
This PR takes the approach of mlir-opt: attempt to print to scientific 6dp notation iff the resulting string is losslessly reproducible, or else print to full precision using repr. This has the advantage of not affecting a vast number of filechecks, mirroring upstream, and not having a flag for either full or reduced precision.

@n-io n-io added enhancement New feature or request core xDSL core (ir, textual format, ...) labels Nov 1, 2024
@n-io n-io self-assigned this Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.12%. Comparing base (5e2f5da) to head (0691a1d).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3381      +/-   ##
==========================================
+ Coverage   90.11%   90.12%   +0.01%     
==========================================
  Files         450      450              
  Lines       56798    56826      +28     
  Branches     5459     5461       +2     
==========================================
+ Hits        51182    51215      +33     
+ Misses       4174     4167       -7     
- Partials     1442     1444       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n-io n-io marked this pull request as ready for review November 1, 2024 18:09
@alexarice
Copy link
Collaborator

I think I've missed a bit of discussion, what's wrong with having this as the default?

@n-io
Copy link
Collaborator Author

n-io commented Nov 1, 2024

I think I've missed a bit of discussion, what's wrong with having this as the default?

I completely agree, but having this as a default requires changing a vast number of tests. Also, I suspect that full-precision is not actually very good for cross-system reproducible file checks, as this is currently breaking in CI but passing on my system.

My preference would be to have a reduced-precision flag and use this on file checks, with full-precision mode being the default.

@n-io
Copy link
Collaborator Author

n-io commented Nov 1, 2024

I've changed the default behaviour to print with full precision.

xdsl/xdsl_opt_main.py Outdated Show resolved Hide resolved
@n-io
Copy link
Collaborator Author

n-io commented Nov 2, 2024

Here's a minimally intrusive suggestion, which takes the same approach of mlir-opt: attempt to print to scientific 6dp notation iff the resulting string is losslessly reproducible, or else print to full precision using repr. This has the advantage of not affecting a vast number of filechecks, mirroring upstream, and not having a flag for either full or reduced precision.

@n-io n-io requested a review from superlopuh November 2, 2024 22:01
Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I feel I would still personally prefer that "simple" floats print in non scientific format, but I guess this is just too large of a filecheck diff

@n-io
Copy link
Collaborator Author

n-io commented Nov 4, 2024

Looks good.

I feel I would still personally prefer that "simple" floats print in non scientific format, but I guess this is just too large of a filecheck diff

It can always be done in the future. I'm not sure about the performance of parsing the str back to a float to ensure there is no precision loss, and printing it straight through repr seems perfectly fine in my opinion. Though it seemed a quick way to ensure not losing precision, which was causing issues to us.

@n-io n-io merged commit d6db473 into main Nov 4, 2024
14 checks passed
@n-io n-io deleted the nicolai/print-full-precision-fp branch November 4, 2024 11:37
lfrenot pushed a commit that referenced this pull request Nov 4, 2024
`mlir-opt` attempts to print floats as 6dp scientific notation, but
round-trips to ensure there is no precision loss and the printed number
will bitwise reproduce exactly. If this fails, it will choose a
different printing method to ensure bit-reproducible printing for a
given precision of fp types.

This PR takes the approach of mlir-opt: attempt to print to scientific
6dp notation iff the resulting string is losslessly reproducible, or
else print to full precision using repr. This has the advantage of not
affecting a vast number of filechecks, mirroring upstream, and not
having a flag for either full or reduced precision.

---------

Co-authored-by: n-io <[email protected]>
lfrenot pushed a commit that referenced this pull request Nov 4, 2024
`mlir-opt` attempts to print floats as 6dp scientific notation, but
round-trips to ensure there is no precision loss and the printed number
will bitwise reproduce exactly. If this fails, it will choose a
different printing method to ensure bit-reproducible printing for a
given precision of fp types.

This PR takes the approach of mlir-opt: attempt to print to scientific
6dp notation iff the resulting string is losslessly reproducible, or
else print to full precision using repr. This has the advantage of not
affecting a vast number of filechecks, mirroring upstream, and not
having a flag for either full or reduced precision.

---------

Co-authored-by: n-io <[email protected]>
@n-io n-io changed the title core: Adding option to print full precision fp core: Print fp losslessly Nov 8, 2024
@n-io n-io changed the title core: Print fp losslessly core: Print fp literals losslessly Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...) enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants