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

Change use of long double to double. #1246

Merged
merged 3 commits into from
Nov 19, 2020

Conversation

brenthuisman
Copy link
Contributor

@brenthuisman brenthuisman commented Nov 17, 2020

Change from long double to double for storing floating point values in modcc.

WSL, and Windows generally, treats long double differently than Linux, which leads to inconsistencies in literal numeric values in code generated by modcc on the two platforms.

Fixes #1245

@halfflat
Copy link
Contributor

halfflat commented Nov 17, 2020

We'd have to change the uses of long double to double in modcc too.

@brenthuisman
Copy link
Contributor Author

What about long long? I think those are OK, but please confirm :)

@halfflat
Copy link
Contributor

I think we can consider changing long long to an explicit width type as something to consider in the scope of a different PR.

Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

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

Does this fix the test on WSL? I thought that the problem was that on WSL the long double was being treated as a double. If so, using double will still create the incorrect result.

How about we instead update the test to test for relative error, and not absolute error? I would like to keep the explicit, more accurate, compile time long double if possible.

@halfflat
Copy link
Contributor

My issue with long double is that it bakes in a platform-dependent numerical interpretation in generated code, making reproducibility harder; it's not just that the specific numerical environment would need to be recreated (if this is indeed possible), but this has to be done on the platform that runs modcc as well as on the execution platform.

It's not too stringent a requirement, I think, that we demand IEEE double precision semantics everywhere for correctness and reproducibility, but expecting long double behaviour to be the same everywhere is unreasonable.

@thorstenhater
Copy link
Contributor

Question: What was the rationale for using long double in the first place?

@bcumming
Copy link
Member

Rationale was that it was at least as accurate as double, likely more so. If we were going to do folding, might as well make it accurate. That was before I knew what I know now.

@brenthuisman
Copy link
Contributor Author

The PR at present does make the unit-modcc test pass on Windows WSL.

@bcumming bcumming merged commit d75a1bc into arbor-sim:master Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WSL: modcc unit test fails
4 participants