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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.) |
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? |
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. |
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 :-) |
Thanks!
clang/test/Driver/aarch64-cpus-1.c | ||
---|---|---|
1 |
Yeah that sounds good. Maybe aarch64-cortex-cpus.c might be a good start, to avoid having too many small files. |
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".
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 |
Looks like these were duplicate tests? Besides armv8.7a vs armv8.7-a