Disable MSVC optimizations for AVX512 GET_CHUNK_MAG #1884
Conversation
WalkthroughThis change adds compiler-specific pragmas around the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (200)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MSVC compiler (VS 17.11.x) incorrectly optimizes the GET_CHUNK_MAG code on older versions. Appears to be resolved in VS 17.13.2. The compiler would optimize the code in such a way that it would cause a decompression failure. It only happens when /Os flag is set.
43e0547 to
665ca3a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1884 +/- ##
============================================
+ Coverage 42.10% 82.52% +40.41%
============================================
Files 71 141 +70
Lines 7353 12672 +5319
Branches 1265 2909 +1644
============================================
+ Hits 3096 10457 +7361
+ Misses 4035 1243 -2792
- Partials 222 972 +750 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| /* MSVC compiler decompression bug when optimizing for size */ | ||
| #if defined(_MSC_VER) && _MSC_VER < 1943 | ||
| # pragma optimize("", off) |
There was a problem hiding this comment.
Does it still work if the first parameter is "t", as it would not disable optimizations when /Ot is enabled.
There was a problem hiding this comment.
I tried it but it does not work right.
There was a problem hiding this comment.
/Ot optimizes for speed, so the comment is then kind of inaccurate...
There was a problem hiding this comment.
Don't quite understand your last comment. That optimization string argument only works if you are starting from enabling optimization, not starting from disabling optimization.
There was a problem hiding this comment.
/Ot and /Os are mutually exclusive, however both also enable for example /Og (common sub-expression elimination). With current patch, it will disable all optimizations and thus degrade performance even if optimizing for speed.
Basically what I was saying was, that disable /Os and enable /Ot instead... might need two pragma lines to do that...
Something like:
# pragma optimize("s", off)
# pragma optimize("t", on)
|
I'm wondering if we could find a lower bound for the bug, too. It'd be great to bookend this check between versions. |
|
That is a lot of work. There is no easy way to switch back to an older version of MSVC like there is with Linux packages. |
We could try to clamp it to specific toolchain. As said reverting to older build of same toolchain version is too much hassle. If both v142 and v143 have the bug then we don't need to worry about v141 or older as those are out of support too. Not sure which toolchain introduced AVX512 support as my CPU does not support it. Visual Studio Installer can install multiple builds of same toolchain but I don't know how CMake can switch between them. |
|
Specify the toolset version: https://cmake.org/cmake/help/v3.25/variable/CMAKE_GENERATOR_TOOLSET.html |
That works once per build directory... Need to use different directory or remove the cache and generated files before switching to another toolchain version/build... |
|
https://x.com/i/grok/share/XwMErXewB6EjJK3X2owak0A6U I don't really have plans to check older versions as this change is limited to one tiny function. If somebody else wants to do it, I would appreciate it, but I don't think it's really necessary. |
|
@nmoinvaz I've seen ms compiler fail in in similar cases with heavy intrisic use. When I tried to debug them, I noticed that by placing some printf's such issues often evaporated. After careful "optimization" of such "fix", it looks like often placing a |
MSVC compiler (VS 17.11.x) incorrectly optimizes the GET_CHUNK_MAG code on older versions. Appears to be resolved in VS 17.13.2. The compiler would optimize the code in such a way that it would cause a decompression failure. It only happens when /Os flag is set.
Summary by CodeRabbit