-
Notifications
You must be signed in to change notification settings - Fork 251
Update fptraits.hpp for gcc/windows/aarch64. #1350
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
Also adds better error checking. Fixes #1348
| // PowerPC extended double precision format (128 bits) | ||
|
|
||
| static_assert(LDBL_MANT_DIG == 113, "Oops, assumption that long double is a 128-bit quantity is incorrect!!"); | ||
|
|
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.
In the PowerPC case should we check both 106 and 113 for LDBL_MANT_DIG? I assume the original code is old enough that 106 was the only option at the time.
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.
Not in the same branch, everything has to go to the correct #if branch otherwise we don't get the correct bits.
|
@mborland I can't seem to get CUDA/SYCL correct: am I correct in thinking that long double is 64 bit on the device and 80-bit on the host? If so is there a good way to configure that? |
That is correct.
I think we'd need to have one set of functions marked host and one set marked device and then let the compiler pick the right one. I'm out of town for the long weekend (MLK day), but I can add those on Tuesday if you'd like. |
Hmm, I suspect that since the functions aren't dependent on any template parameter, the static_assert will be triggered irrespective of whether the function is called or not. |
I guess I was thinking about moving the The rub is then the tag is wrong on device? |
|
Actually, even easier is removing the check for GPU platforms. I was wrong and |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1350 +/- ##
========================================
Coverage 95.29% 95.29%
========================================
Files 814 814
Lines 67422 67422
========================================
Hits 64249 64249
Misses 3173 3173
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Looks like this one is good to go @mborland ? |
Yes. In it goes. |
|
Oh geez, literally 5 minutes after this merge, my builds on |
|
I could add some lines around line 273 to pick up |
|
Around lines 273, I could add which will NOT pick up |
|
Thanks for this @ckormanyos , just thinking out loud here, I wonder if it's possible to change the default branch to only check long double sizes if that function is actually used (we would need to make it a template somehow)? Otherwise we risk this issue coming up again and again as new platforms get used. |
You know @jzmaddock, I still struggled with this one. I got it working locally, but still had to disable some tests in my embedded CI/CD. So I haven't fully understood the testing logic yet. At this moment, I think we have something that passes CI/CD on Boost.Math. And I can work successfully locally with it. But indeed, I think the logic could and probably will be refined. I just got in a super-hurry yesterday and put in the minimal PP-stuff I needed for my local builds. It can surely be improved down the road. |
|
I'm wondering if we can't remove much of the use? For instance |
Perhaps, but in general they're pretty efficient and portable. They also correctly detect NaN's in situations where some |
|
This broke the PPC64LE QEMU runners from Boost.CI: |
Hi @mborland, good catch. Ugghhh what exactly broke the 64-bit Power PC QEMU runners? Were my very recent changes for |
|
The entire PR, since you didn't touch this path. I'm thinking it may be time to roll this back. There's going to be lots of other platforms we don't account for I'm sure. I'll defer to @jzmaddock |
That is a good consideration Matt (@mborland). It's one of those things, ... do you roll back or press on? I have Boost.Math running on embedded For the record, I can live with this PR. So if we get the failed runners on PPC-64 working, that would be sufficient for me at the moment. Cc: @jzmaddock |
Where can I see these running? I'm interested in the output from math_info.cpp on these platforms if possible? Meanwhile, see if #1354 helps. |
Here is an example: https://github.com/cppalliance/decimal/actions/runs/21370721961/job/61514255669
I'll have to see if I can get these containers running locally to give it a shot. It's pulling in math from develop in the CI.
|
Also adds better error checking.
Fixes #1348