This is an archive of the discontinued LLVM Phabricator instance.

[Sema][SVE2] Move/simplify Sema testing for SVE2 ACLE builtins
ClosedPublic

Authored by RosieSumpter on Apr 12 2022, 6:26 AM.

Details

Summary

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.

Diff Detail

Event Timeline

RosieSumpter created this revision.Apr 12 2022, 6:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
RosieSumpter requested review of this revision.Apr 12 2022, 6:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 6:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
paulwalker-arm added inline comments.Apr 12 2022, 6:55 AM
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?

RosieSumpter added inline comments.Apr 12 2022, 8:18 AM
clang/test/Sema/aarch64-acle-sve2-aes.c
1 ↗(On Diff #422213)

Will do.

5–9 ↗(On Diff #422213)

Will do.

Matt added a subscriber: Matt.Apr 14 2022, 2:07 PM
RosieSumpter retitled this revision from Work in progress: [Sema][SVE] Move sema testing for SVE2-AES ACLE builtins to [WIP][Sema][SVE] Move/simplify Sema testing for SVE ACLE builtins.
RosieSumpter edited the summary of this revision. (Show Details)

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.

Added REQUIRES: aarch64-registered-target line to each new test file.

sdesmalen accepted this revision.Apr 27 2022, 5:58 AM

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

This revision is now accepted and ready to land.Apr 27 2022, 5:58 AM
paulwalker-arm added inline comments.Apr 27 2022, 4:15 PM
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?

RosieSumpter added inline comments.Apr 28 2022, 1:19 AM
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

RosieSumpter marked an inline comment as done.Apr 28 2022, 1:26 AM
RosieSumpter added inline comments.
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_lane.cpp
152–153

It's already on line 86!

paulwalker-arm added inline comments.Apr 28 2022, 3:42 AM
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.

RosieSumpter marked 2 inline comments as done.
RosieSumpter retitled this revision from [WIP][Sema][SVE] Move/simplify Sema testing for SVE ACLE builtins to [Sema][SVE2] Move/simplify Sema testing for SVE2 ACLE builtins.
RosieSumpter edited the summary of this revision. (Show Details)
  • Removed EXPAND... macro
  • Added missing tests
  • Alternate between 0 and 180 argument for test_90_270()
RosieSumpter added inline comments.Apr 28 2022, 4:30 AM
clang/test/Sema/aarch64-sve2-intrinsics/acle_sve2_imm_n.cpp
26

I've removed the macro - agree that it ruins the flow

paulwalker-arm accepted this revision.Apr 28 2022, 4:54 AM
clang/test/CodeGen/aarch64-sve2-intrinsics/negative/acle_sve2_sri.c