tmatheson accepted this revision. tmatheson added a comment. This revision is now accepted and ready to land.
Thanks for picking this up, LGTM ================ Comment at: clang/test/Driver/aarch64-archs.c:296 -// NO-LS64-NOT: "-target-feature" "+ls64" -// LS64: "-target-feature" "+ls64" - ---------------- Looks like these were duplicate tests? Besides `armv8.7a` vs `armv8.7-a` ================ Comment at: clang/test/Driver/aarch64-cpus.c:2 +// Check target CPUs are correctly passed. +// TODO: The files should be split up by categories, e.g. by architecture versions, to avoid excessive test // times for large single test files. ---------------- tyb0807 wrote: > simon_tatham wrote: > > 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.) > 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? ================ Comment at: clang/test/Driver/aarch64-ras.c:14 +// RAS is on by default for v8.2a, but can be disabled by +noras +// FIXME: in the current implementation, RAS is not on by default at all for v8.2a (the test says it all...) +// RUN: %clang -target aarch64 -march=armv8.2a -### -c %s 2>&1 | FileCheck -check-prefix=V82ARAS %s ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121093/new/ https://reviews.llvm.org/D121093 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
