Enable warning C4242 and treat warnings as errors for Visual C++. #1768
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1768 +/- ##
========================================
Coverage 83.03% 83.03%
========================================
Files 135 135
Lines 10304 10310 +6
Branches 2784 2785 +1
========================================
+ Hits 8556 8561 +5
+ Misses 1055 1054 -1
- Partials 693 695 +2 ☔ View full report in Codecov by Sentry. |
b77611c to
9875a8b
Compare
bc00bd8 to
4a776c8
Compare
fb1f0b4 to
d2afadc
Compare
| #if defined(_MSC_VER) && !defined(_MM_K0_REG8) | ||
| # undef _mm512_extracti64x4_epi64 | ||
| # define _mm512_extracti64x4_epi64(v1, e1) _mm512_maskz_extracti64x4_epi64(UINT8_MAX, v1, e1) | ||
| # undef _mm512_set1_epi16 | ||
| # define _mm512_set1_epi16(e1) _mm512_maskz_set1_epi16(UINT32_MAX, e1) | ||
| # undef _mm512_maddubs_epi16 | ||
| # define _mm512_maddubs_epi16(v1, v2) _mm512_maskz_maddubs_epi16(UINT32_MAX, v1, v2) |
There was a problem hiding this comment.
So is this actually a bad definition in their intrinsic that causes a partial mask, or is this just something silly like Microsoft defines this as a macro with -1 as the mask (which is technically correct but an overly pedantic compiler might try to complain about it)?
There was a problem hiding this comment.
@KungFuJesus Microsoft forgot to define the mask for half of the variants, and instead used mask for twice the big bit width and assumed their own compiler will truncate it without warning. So basically there was only _MM_K0_REG16 and _MM_K0_REG64 defined.
There was a problem hiding this comment.
Ah, I see, so it sounds like they weren't quite anticipating AVX512BW and just dropped in the 16 bit masks instead. Which still result in correct behavior, but the implicit truncation causes their pedantic warnings to trigger. It is still silly but I guess functionally this is fine.
There was a problem hiding this comment.
@KungFuJesus Microsoft likes to do things their own way and not copy how others have done the same thing... I don't think they anticipated that developers start to use higher warning levels than 3 on production code or that developers want to treat all warnings as errors, even though they anticipated that developers want to silence some warnings or to move some warnings to different level.
| #endif // defined(_MSC_VER) && _MSC_VER < 1914 | ||
|
|
||
| /* Visual C++ toolchains before v142 have constant overflow in AVX512 intrinsics */ | ||
| #if defined(_MSC_VER) && defined(__AVX512F__) && !defined(_MM_K0_REG8) |
There was a problem hiding this comment.
Is this the most reliable preprocessor definition for this (_MM_K0_REG8)? Shouldn't we use an MSC_VER comparison?
There was a problem hiding this comment.
_MSC_VER doesn't show the toolchain or SDK version, only the IDE version... In this case it was using Visual C++ 2019 frontend, but the toolchain was from Visual C++ 2015 (v140) or Visual C++ 2017 (v141), so the used SDK (headers) was broken.
There was a problem hiding this comment.
Yeah and given the context above I say this should be fine. Basically, they don't have the 8 bit mask registers defined.
There was a problem hiding this comment.
I really didn't want to diff all the available SDK versions to see if there is a header that defines the SDK version and which SDK version did fix each of the broken intrinsic macros... Making such complex tests would have warranted extensive commenting and might have slowed down preprocessing and compiling the code by quite a lot.
Level 4 warning C4242 is
'identifier': conversion from 'type1' to 'type2', possible loss of data, which is usually result of missing truncating cast or incorrect type used in cast or expression.See discussion in #1762. Rationale for treating warnings as errors is that most developers use Linux or other Unix-compatible systems and only small fraction have Windows to test any changes.