This is an archive of the discontinued LLVM Phabricator instance.

[clang][RISCV][NFC][test] Move riscv-abi.cpp and riscv{32,64}-*abi.c tests to use update_cc_test_checks.py
ClosedPublic

Authored by asb on Sep 16 2022, 8:08 AM.

Details

Summary

This patch implements an initial step towards refactoring our ABI tests (moving them to update_cc_test_checks.py), with future steps and motivations described in the note below.

Old summary:

I think our current set of ABI test files is organised about as well as it can be when relying on manual editing, but maintenance is error-prone and tedious. Having files containing tests that have the same signature for the ABIs indicated in the filename simplifies some things, but can create additional work when adding a new test that has behaviour that's worth testing across multiple ABI combinations. It's also not easy to tell at a glance whether a given function signature is tested across a sufficient set of ABIs, as you may need to look in multiple files to check for this.

This patch isn't ready for committing, and I don't propose committing it in its current form. As I've had to do some work on update_cc_test_checks.py to make it suitable for generating our ABI tests (see D133943) and will need to do further work to finalise this work, I wanted to get some feedback on the proposed direction.

This patch demonstrates the proposed direction I'd like to take these ABI tests in, with some caveats:

  • Due to an update_cc_test_checks.py bug I haven't spent the time to track down and fix yet, I have to include at least one line from the function body for it to merge CHECK lines with the same content. I'd anticipate fixing this if we're agreed this is the right direction.
  • riscv-abi.cpp is now generated using update_cc_test_checks.py, and also demonstrates the style of tests I'd like to move to - where all ABI variants are tested in a single file (potentially split by language features if it becomes too large) and we rely on update_cc_test_checks.py merging common CHECK lines for us.
  • The riscv32-*abi.c tests are all regenerated using update_cc_test_checks.py. This represents the first step of my proposed changes - the next step would be to combine into one (or a small number of files based on language features / aspect of the ABI rather than target ABI variant) and use multiple RUN lines with overlapping check prefixes much like riscv-abi.cpp
    • It _probably_ makes sense to combine riscv32-*abi.c and riscv64*-abi.c tests - I've just made a start with riscv32* so as to get something for quick feedback

So - how do people feel about this direction? Potential options:

  • Don't change existing tests and don't move to update_cc_test_checks
  • Convert current tests to update_cc_test_checks but don't pursue any further refactoring/merging
  • Convert current tests to update_cc_test_checks and then pursue something like I suggest above, combining more tests in the style of riscv-abi.cpp

EDIT: To take a step back a bit, the overall goal is to make it less burdensome to add new hand-written ABI tests, to improve our coverage for potential corner cases. There are also things we could do to try to perform more automated ABI testing, but I think that's orthogonal.

Diff Detail

Event Timeline

asb created this revision.Sep 16 2022, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 8:08 AM
asb requested review of this revision.Sep 16 2022, 8:08 AM
asb edited the summary of this revision. (Show Details)Sep 16 2022, 8:11 AM
kito-cheng accepted this revision.Oct 27 2022, 12:28 AM

LGTM, manually update such testcase is really painful!

This revision is now accepted and ready to land.Oct 27 2022, 12:28 AM

Oh, it's still [WIP/RFC], but anyway that's really good way to go.

asb updated this revision to Diff 484207.Dec 20 2022, 3:43 AM
asb retitled this revision from [clang][RISCV][NFC][WIP/RFC] Move riscv-abi.cpp and riscv32-*abi.c tests to use update_cc_test_checks.py to [clang][RISCV][NFC][WIP/RFC] Move riscv-abi.cpp and riscv{32,64}-*abi.c tests to use update_cc_test_checks.py.
asb edited the summary of this revision. (Show Details)

Sync with my downstream patch by adding in the RV64 ABI test changes.

asb updated this revision to Diff 501466.Mar 1 2023, 5:10 AM
asb retitled this revision from [clang][RISCV][NFC][WIP/RFC] Move riscv-abi.cpp and riscv{32,64}-*abi.c tests to use update_cc_test_checks.py to [clang][RISCV][NFC][test] Move riscv-abi.cpp and riscv{32,64}-*abi.c tests to use update_cc_test_checks.py.

Rebase on top of D144963 (it looks like there's a clear path forward on this now, so I'm expecting to be able to land this relatively soon).

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2023, 8:45 AM
Herald added a subscriber: jobnoorman. · View Herald Transcript