This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Add checks for no warnings in SVE tests
ClosedPublic

Authored by david-arm on May 28 2020, 4:47 AM.

Details

Summary

There are now quite a few SVE tests in LLVM and Clang that do not
emit warnings related to invalid use of EVT::getVectorNumElements()
and VectorType::getNumElements(). For these tests I have added
additional checks that there are no warnings in order to prevent
any future regressions.

Diff Detail

Event Timeline

david-arm created this revision.May 28 2020, 4:47 AM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

I guess we could do this as a temporary measure, if you think it's useful; eventually, of course, the codepath to print the warning will go away.

Since you checked this, how many tests do still print warnings?

Hi @efriedma, at least amongst all the tests in llvm/test/CodeGen/AArch64/sve-* there are still 66 with warnings. @sdesmalen and I discussed this and our reason for adding checks for warnings is mainly to do with the fact we are still fixing up cases and implementing SVE codgen support, so there is a chance that somewhere we'll introduce a regression without realising it. That's the main rationale for this, but I do realise that ultimately getVectorNumElements() will be deprecated. I also realised that the warnings I've added to the clang tests aren't that useful unless we're compiling them to real assembly so I may add some new RUN lines for those.

Okay.

Thinking about it a bit more, I think I'd prefer using a pattern where we redirect stderr to a file, and CHECK it separately, to make sure we don't miss a warning because it got interleaved into the middle of stdout.

david-arm updated this revision to Diff 268080.Jun 3 2020, 1:24 AM
sdesmalen added inline comments.Jun 10 2020, 4:27 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_abd.c
4

Just thinking out loud here; we don't need to test for the specific warnings, because LLVM doesn't emit any other warnings and any Clang's warnings are otherwise caught by -Werror.

4

Can you add a comment to these checks, so that if someone breaks it (because their code causes a new warning), they know where to look?

david-arm updated this revision to Diff 271038.Jun 16 2020, 4:15 AM

Hi @sdesmalen @efriedma , hopefully I've addressed your review comments with my latest patch!

This revision is now accepted and ready to land.Jun 16 2020, 2:10 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 18 2020, 8:06 AM
thakis added inline comments.
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_adda.c
3 ↗(On Diff #271591)

This needs a -o -, or better a -o /dev/null instead of the piping to /dev/null. As-is, this creates a .s file in the test dir next to this file which on the next run is executed as a test that's unresolved. (Add a rm -f %S/acle_sve_adda.s to clean up bots, with a FIXME to remove this after a few days / weeks.