-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
BLD: fix uninitialized variable warnings from simd/neon/memory.h #25459
Conversation
The warning was this, repeated many times: ``` In file included from ../numpy/_core/src/common/simd/neon/neon.h:76: ../numpy/_core/src/common/simd/neon/memory.h:56:56: warning: variable 'a' is uninitialized when used here [-Wuninitialized] a = vld1q_lane_s32((const int32_t*)ptr, a, 0); ^ ``` [skip cirrus] [skip azp]
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.
As far as I can see, this emits the same output, initializing it whether you like it or not, so I would go ahead and merge this to fix the warning.
Ideally, we wouldn't have to initialize it, as we immediately overwrite it. Highway supports this for specific sections using macros; is that right @jan-wassenberg?
https://github.com/google/highway/blob/master/hwy/ops/arm_neon-inl.h#L937-L949
Thanks for the review @Mousius. Re macros with diagnostic pragmas: those are never really portable, and from a quick browse of the Highway code it looks pretty specific to MSVC and GCC. That means that you get swamped with warnings on compilers that aren't handled; Clang already tends to complain about the GCC macros it doesn't know about. So it's best to avoid these unless the performance impact is really large. |
@rgommers I guess the other option is to disable it blanketly for SIMD stuff then 🤔 |
My bad. It's a false warning, it can be avoided by splats the first loaded element rather than initializing with zero, e.g.: -int32x4_t a = vdupq_n_s32(0);
+int32x4_t a = vdupq_n_s32(*ptr);
-a = vld1q_lane_s32((const int32_t*)ptr, a, 0); |
Ah thanks Sayed. Do you want to open a follow-up PR? |
Let me check the compiler behavior under optimization level 3 first, I think the compiler can optimize it out. |
A quick comment on pragmas: clang/gcc are largely the same, in rare cases they differ, for example the 'maybe-uninitialized' warning from the code you linked (only GCC has that one). |
No difference between the three versions the current, the previous, and the suggested one on clang, for GCC the current and the previous perform the same! initialize the returned vector with zero before loading. So it's totally fine to keep it as-is. |
Awesome:) |
The warning was this, repeated many times:
I think the way I initialized the variable is idiomatic, but please double check (@Mousius or @seiko2plus?) since I'm not too familiar with this stuff.
Closes gh-25461