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.

Sign up for GitHub

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

Jump to bottom

dialects: (builtin) mimic mlir floating point precision for printing and parsing #3607

Merged
merged 2 commits into from
Dec 12, 2024
Merged

dialects: (builtin) mimic mlir floating point precision for printing and parsing #3607

merged 2 commits into from
Dec 12, 2024

Conversation

jorendumoulin
Copy link
Collaborator

I'm running into some precision issues when printing and parsing floats after they have been packed/unpacked.
This PR tries to resolve the issues with printing and parsing, trying to mimic MLIR behaviour as much as possible.

jorendumoulin
jorendumoulin commented Dec 9, 2024
View reviewed changes
xdsl/printer.py
Comment on lines -484 to +487
float_str = f"{value:.6e}"
if float(float_str) == value:
float_str = f"{value:.5e}"
index = float_str.find("e")
float_str = float_str[:index] + "0" + float_str[index:]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mlir does this interesting thing where it rounds to 5 digits after the comma but still prints the 6th 0

jorendumoulin
jorendumoulin commented Dec 9, 2024
View reviewed changes
xdsl/printer.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this could hurt performance for a large amount of floats, which can possibly be resolved by just dumping the hex value of the bytes representation as it is done for arrays > ~100ish elements in MLIR:

module {
  %cst = arith.constant dense<"0xC3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43C3F508409EEF0842F5DBC141E73B6A43C76B6A43"> : tensor<180xf32>
}

@jorendumoulin jorendumoulin added the dialects Changes on the dialects label Dec 9, 2024
@jorendumoulin
Copy link
Collaborator Author

Before I start fixing the all the filechecks that are affected by this, could I get your opinion on whether this should be implemented like this? @superlopuh @alexarice @compor @math-fehr

@alexarice
Copy link
Collaborator

My understanding is that the current float parsing/printing was changed recently so that it always roundtripped correctly. Was there a bug in this implementation? I'm not 100% it's worth changing how all the floats are represented. (As a side note I'm not exactly sure if it's even desirable to use scientific notation everywhere for floats, but it's possible I'm missing something.)

@jorendumoulin
Copy link
Collaborator Author

jorendumoulin commented Dec 9, 2024 •
edited
Loading

My understanding is that the current float parsing/printing was changed recently so that it always roundtripped correctly. Was there a bug in this implementation? I'm not 100% it's worth changing how all the floats are represented. (As a side note I'm not exactly sure if it's even desirable to use scientific notation everywhere for floats, but it's possible I'm missing something.)

Not really a bug, just diverging between behaviour between mlir/xdsl:

for an input

arith.constant 1.1 : f16
arith.constant 3.1415 : f32
arith.constant 3.141592 : f32
arith.constant 3.1415 : f64
arith.constant 3.141592 : f64

xdsl right now returns:

builtin.module {
  %0 = arith.constant 1.100000e+00 : f16
  %1 = arith.constant 3.141500e+00 : f32
  %2 = arith.constant 3.141592e+00 : f32
  %3 = arith.constant 3.141500e+00 : f64
  %4 = arith.constant 3.141592e+00 : f64
}

and mlir (and xdsl with this PR):

builtin.module {
  %0 = arith.constant 1.099610e+00 : f16
  %1 = arith.constant 3.141500e+00 : f32
  %2 = arith.constant 3.14159203 : f32
  %3 = arith.constant 3.141500e+00 : f64
  %4 = arith.constant 3.1415920000000002 : f64
}

The main problem with the current version of xDSL i have right now is when things are backed with bytes storage vs as FloatAttr, for example for 2.1: f32:

builtin.module {
  %0 = arith.constant {"test" = array<f32: 2.0999999046325684>} 0 : i64
  %1 = arith.constant 2.100000e+00 : f32
}

@compor
Copy link
Collaborator

compor commented Dec 9, 2024

I don't have any argument to shoot this down ATM, and I'm in general in favor of such convergence.

@jorendumoulin jorendumoulin marked this pull request as ready for review December 9, 2024 17:47
@superlopuh
Copy link
Member

Yep this seems like a great change

@codecov Codecov
Copy link

codecov bot commented Dec 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.15%. Comparing base (e719c7f) to head (b7bbe6c).
Report is 7 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3607      +/-   ##
==========================================
- Coverage   90.48%   90.15%   -0.33%     
==========================================
  Files         472      463       -9     
  Lines       59276    58737     -539     
  Branches     5637     5631       -6     
==========================================
- Hits        53633    52952     -681     
- Misses       4205     4346     +141     
- Partials     1438     1439       +1

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

superlopuh
superlopuh approved these changes Dec 10, 2024
View reviewed changes
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.

Would be good to double-check with @math-fehr

xdsl/dialects/builtin.py Outdated Show resolved Hide resolved
@jorendumoulin
dialects: (builtin) mimic mlir floating point precision for printing …
9c247d5 
…and parsing

format

fix filecheck

update tests

fixing filechecks

remaining non-mlir filechecks

small x

formatting

fix pytest

remaining lowercase x

another remaining lowercase x

remove try/except

list to tuple
@jorendumoulin jorendumoulin force-pushed the joren/print-float-precision branch from 2c7c783 to 9c247d5 Compare December 11, 2024 10:33
@jorendumoulin jorendumoulin mentioned this pull request Dec 11, 2024
Open
@compor compor self-requested a review December 11, 2024 10:38
math-fehr
math-fehr approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@math-fehr math-fehr 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 to me!

@jorendumoulin jorendumoulin merged commit 7e38685 into main Dec 12, 2024
15 checks passed
@jorendumoulin jorendumoulin deleted the joren/print-float-precision branch December 12, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@math-fehr math-fehr math-fehr approved these changes

@superlopuh superlopuh superlopuh approved these changes

@compor compor Awaiting requested review from compor

Assignees

@jorendumoulin jorendumoulin

Labels
dialects Changes on the dialects
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jorendumoulin @alexarice @compor @superlopuh @math-fehr