Currently for SVE2 ACLE builtins, single tests are used to verify both
clang code generation (when the feature is available) and semantic
error/warning messages (when the feature is unavailable). This
patch moves the semantic testing for the values of immediate
arguments into dedicated Sema tests.
Details
- Reviewers
sdesmalen paulwalker-arm efriedma
Diff Detail
Event Timeline
clang/test/Sema/aarch64-acle-sve2-aes.c | ||
---|---|---|
1 ↗ | (On Diff #422213) | Relying on the implicit declaration... warning is potentially problematic and what triggered this work in the first place. Can you make the new Sema tests c++ ones instead. Doing this means we can catch errors like error: use of undeclared identifier 'svaesd_u8' instead of a c99 warning. |
5–9 ↗ | (On Diff #422213) | The original versions of these tests used the SVE_ACLE_FUNC macro so that the overloaded builtin names are also protected. Can you do likewise for the new tests? |
This patch is now for moving/simpifying sematic testing for immediate arguments of the builtins. At the moment this is just for the SVE2 intrinsics so that the structure of the new tests can be checked, the tests for the SVE intrinsics will be added to this patch later.
Semantic testing for the feature flag (i.e. 'use of undeclared identifier...' error messages) will be in a separate patch.
Thanks for this @RosieSumpter. If you remove the 'WIP' and s/SVE/SVE2/ from the title and description, I'm happy to accept. The negative tests for the SVE(1) intrinsics can be added as a separate patch.
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_lane.cpp | ||
---|---|---|
152–153 | nit: there is also a svcdot_lane,_s32 |
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_n.cpp | ||
---|---|---|
26 | I've not seen this before, presumably it's short hand instead of needing to repeat multiple identical expected-error check lines? If so, is it worth using this throughout the test files and essentially only require one expected-error per function or does this only work here because the EXPAND... macro emits its three function calls on the same line? |
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_n.cpp | ||
---|---|---|
26 | Yes it lets you specify how many times you expect the diagnostic to appear, but as you said it only works when the diagnostics are emitted on the same line so I'm not sure there's a way to reduce the number of expected-error lines any more than this |
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_lane.cpp | ||
---|---|---|
152–153 | It's already on line 86! |
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_n.cpp | ||
---|---|---|
26 | OK, thanks for checking. To be honest I'm not sure why we need the EXPAND_XZM_FUNC macro given SVE_ACLE_FUNC worked fine before. To my eye it kind of ruins the flow, but hey-ho I'll not worry about it. Assuming I've not screwed up I think you're missing tests for SVE_ACLE_FUNC(svrshrnb,_n_s16,,) and SVE_ACLE_FUNC(svrshrnt,_n_s16,,). | |
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_rotation.cpp | ||
18–41 | I know we cannot test every number but 180 seems like a reasonable mistake for people to make given the other complex number instructions so perhaps alternate between 0 and 180 to give a little more coverage without increasing the number of lines. |
- Removed EXPAND... macro
- Added missing tests
- Alternate between 0 and 180 argument for test_90_270()
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_n.cpp | ||
---|---|---|
26 | I've removed the macro - agree that it ruins the flow |
nit: there is also a svcdot_lane,_s32