This is an archive of the discontinued LLVM Phabricator instance.

[Clang][NeonEmitter] emit ret decl first for -Wdeclaration-after-statement
ClosedPublic

Authored by nickdesaulniers on Mar 21 2022, 2:55 PM.

Details

Summary

The generated arm_neon.h header isn't -Wdeclaration-after-statement
compliant when targeting -mbig-endian. Update the generator to declare
the return value, if any, first before any other arguments that might
need to be "reversed" from little endian to big.

Another approach would have been to try to ignore this warning in system
headers, though that might not be precise for tokens involved in macro
expansion. See also: https://reviews.llvm.org/D116833#3236209.

Link: https://github.com/ClangBuiltLinux/linux/issues/1603
Fixes: https://github.com/llvm/llvm-project/issues/54062

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 2:55 PM
nickdesaulniers requested review of this revision.Mar 21 2022, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 2:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • rebase on main, don't mass refomat tests

This passes through all my ARCH=arm and ARCH=arm64 build and boot tests on next-20220321 with the instances of -Wdeclaration-after-statement resolved and no new instances of any other warnings.

Can you post a bit of the header diff before/after? I think I know what it looks like but just to be sure.

clang/test/Sema/arm-neon-decl-after-stmt.c
4

Does armebv7 work when you only have aarch64?

It probably does just by virtue of aarch64 not knowing what v7 is and defaulting to v8 but double check what happens there.

If the emitter is used for both arm and aarch64 then testing one would be acceptable. Since you can't really do an if has arm do this else in a lit test like this.

clang/utils/TableGen/NeonEmitter.cpp
1855

Comment the reason for doing the return var first

nickdesaulniers marked an inline comment as done.
  • add comments, remove test requirement on 64b

Can you post a bit of the header diff before/after? I think I know what it looks like but just to be sure.

Before:
#define noswap_splat_lane_f16(p0,p1) extension__ ({ float16x4_t s0 = p0; float16x4_t ret; ret = (float16x4_t) builtin_neon_splat_lane_v((int8x8_t)s0, p1, 8); ret; })
After:
#define noswap_splat_lane_f16(p0,p1) extension__ ({ float16x4_t ret; float16x4_t s0 = p0; ret = (float16x4_t) builtin_neon_splat_lane_v((int8x8_t)s0, p1, 8); ret; })

Before:
#define vld1_lane_f32(p0,p1,p2) extension__ ({ float32x2_t s1 = p1; float32x2_t rev1; rev1 = builtin_shufflevector(s1, s1, 1, 0); float32x2_t ret; ret = (float32x2_t) builtin_neon_vld1_lane_v(p0, (int8x8_t)rev1, p2, 9); ret = builtin_shufflevector(ret, ret, 1, 0); ret; })
After:
#define vld1_lane_f32(p0,p1,p2) extension__ ({ float32x2_t ret; float32x2_t s1 = p1; float32x2_t rev1; rev1 = builtin_shufflevector(s1, s1, 1, 0); ret = (float32x2_t) builtin_neon_vld1_lane_v(p0, (int8x8_t)rev1, p2, 9); ret = builtin_shufflevector(ret, ret, 1, 0); ret; })

nickdesaulniers marked an inline comment as done.Mar 22 2022, 9:54 AM
DavidSpickett accepted this revision.Mar 23 2022, 2:35 AM

This LGTM with the test only requiring Arm.

Please mention in the commit that it is possible to ignore this warning in system headers, but we chose not to because of potential false positives.

clang/test/Sema/arm-neon-decl-after-stmt.c
4

Maybe this is phabricator showing me the diff strangely, but it seems like you removed the aarch64 requirement from a different test instead of this one.

This revision is now accepted and ready to land.Mar 23 2022, 2:35 AM
nickdesaulniers marked an inline comment as done.
  • fix incorrect REQUIRES
nickdesaulniers edited the summary of this revision. (Show Details)Mar 23 2022, 9:30 AM
This revision was landed with ongoing or failed builds.Mar 23 2022, 9:41 AM
This revision was automatically updated to reflect the committed changes.