FIX: avoid overflow on overflow check for Mac M1 #945
Conversation
|
@mborland @jzmaddock Didn't realize the changes would be similar for all issues at the start, so consolidating all these small PRs into a single one |
| @@ -647,7 +652,9 @@ namespace detail { | |||
| } | |||
| else if (result > max) | |||
| { | |||
| T diff = ((fabs(max) < 1) && (fabs(result) > 1) && (tools::max_value<T>() / fabs(result) < fabs(max))) ? T(1000) : T(result / max); | |||
| const T amax = fabs(max); | |||
| volatile const T aresult = fabs(result); // volatile: force compiler to honor data-dependency in chained bool exprs below | |||
There was a problem hiding this comment.
You should really only need const volatile in a shared memory environment. Do you still get overflow errors with just const?
There was a problem hiding this comment.
Yes, I tried all the variations I could think of. What also worked was predclaring the amax and aresult and switching around tools::max_value<T>() / fabs(result) < fabs(max) to tools::max_value<T>() / amax < aresult, but I that was only suppressing it in this case for a specific relationship between result and max, so the volatile in my mind makes it a little more generic
|
I believe SciPy is using this image and seeing the errors. It's about 9 months old and I wonder if updating to the latest may avoid these (compiler?) issues/bugs |
Might be worth updating. I am using macOS Ventura with Clang 14.0.0 on an M1 MacBook Pro and see none of the errors. Anyone on Ventura won't even have the option for the Clang 13 series. |
|
I confess I'm liking this less and less: the code appears to be correct, something somewhere is generating incorrect code if spurious overflow is happening on a code branch that should never be taken. Plus volatile plays absolute havoc with compiler optimizers. I don't as it happens mind at all if this is the full extent of the issue, but I have a hunch that we're about to take a deep dive through a very deep rabbit hole, as the library is choc full of logic like this. So I guess the questions are:
|
Testing this out now
Sounds like neither of you have tried on this specific version of MacOS? I also notice it only shows up when
I wasn't able to find anything upstream, I'll submit an issue |
If you are able to make an extremely minimal reproducer like the GCC Bugzilla requires @NAThompson can route it to the right people at Apple. |
Minimal reproducer (not sure how it compares to GCC Bugzilla) is in the upstream llvm-project issue: llvm/llvm-project#60695 (comment) |
@mborland The issue was rejected by Clang maintainers, I'll try looking up my Apple ID and submitting an issue there |
|
Since the issue seems to be fixed in newer macOS I wouldn't be surprised if they don't investigate the bug either. |
[number]*tools::max_value<T>()to prevent overflows on Apple M1 processors