This is an archive of the discontinued LLVM Phabricator instance.

[Driver][AArch64] Split up aarch64-cpus.c test further
ClosedPublic

Authored by tyb0807 on Mar 7 2022, 2:21 AM.

Details

Summary

This is the continuation of https://reviews.llvm.org/D120875, where
aarch64-cpus-2.c is further splitted and renamed to better reflect the
tests.

Diff Detail

Event Timeline

tyb0807 created this revision.Mar 7 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 2:21 AM
tyb0807 requested review of this revision.Mar 7 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 2:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
simon_tatham added inline comments.Mar 7 2022, 2:46 AM
clang/test/Driver/aarch64-cpus-1.c
1

Tiniest nit ever: what files is this comment talking about?

(In the previous version, it followed on from a previous comment that gave the name of the file containing other half of the tests in question. Now it doesn't, so the wording is no longer clear.)

tyb0807 marked an inline comment as done.Mar 7 2022, 3:03 AM
tyb0807 added inline comments.
clang/test/Driver/aarch64-cpus-1.c
1

This is for just in case we want to split up this file (aarch64-cpus.c) further, into smaller files such as aarch64-cortex-a53.c, aarch64-cortex-a57.c, etc. Do you think we still need to do this, or this file looks ok to you in its current state?

tmatheson accepted this revision.Mar 7 2022, 3:36 AM

Thanks for picking this up, LGTM

clang/test/Driver/aarch64-cpus-1.c
1
clang/test/Driver/aarch64-cpus-2.c
296

Looks like these were duplicate tests? Besides armv8.7a vs armv8.7-a

clang/test/Driver/aarch64-ras.c
14

The RAS test changes are the only functional change to this review, which is otherwise moving lines around and changing comments. I would keep the original tests even if they're wrong, so that the history remains clearer. Alternatively describe the RAS changes in the commit message and description.

This revision is now accepted and ready to land.Mar 7 2022, 3:36 AM
simon_tatham accepted this revision.Mar 7 2022, 4:07 AM
simon_tatham added inline comments.
clang/test/Driver/aarch64-cpus-1.c
1

I do quite like the idea of a whole set of aarch64-cortex-<foo>.c files, for the same reason I mentioned in D120875: it means that when someone is implementing support for the next CPU, they'll look at the existing test collection and naturally add a new file alongside all the others, instead of naturally appending another stanza to this file which will eventually grow too big again.

But like the previous patch, I won't block you making this much improvement just because you haven't made even more improvement :-)

fhahn added a comment.Mar 7 2022, 4:22 AM

Thanks!

clang/test/Driver/aarch64-cpus-1.c
1

I do quite like the idea of a whole set of aarch64-cortex-<foo>.c files

Yeah that sounds good. Maybe aarch64-cortex-cpus.c might be a good start, to avoid having too many small files.

tyb0807 updated this revision to Diff 413436.Mar 7 2022, 5:13 AM
tyb0807 marked an inline comment as done.

Further split aarch64-cpus-1.c

Yeah that sounds good. Maybe aarch64-cortex-cpus.c might be a good start, to avoid having too many small files.

Oops, I went ahead and split to many small cortex test files. Not really sure what would be better. What do you think @simon_tatham @fhahn @tmatheson ?

I like this version! This definitely says to me "nobody is going to just thoughtlessly append to an existing file".

tyb0807 marked 2 inline comments as done.Mar 7 2022, 5:24 AM
tyb0807 added inline comments.
clang/test/Driver/aarch64-cpus-2.c
296

Hmm, the first one tests that LS64 is not on by default on armv8.7a, and the second tests that armv8.7a+ls64 enables it. Why these are duplicates?

If you mean L276/277, I guess the intent is to test both syntaxes that we support

clang/test/Driver/aarch64-ras.c
14

Actually, I added the FIXME because I don't want to fix the test in this commit. There's no functional change to this review I think

This revision was automatically updated to reflect the committed changes.
tyb0807 marked 2 inline comments as done.
fhahn added a comment.May 20 2022, 2:14 AM

Thanks for breaking this up!