This is an archive of the discontinued LLVM Phabricator instance.

Do not append exit status to reference_output files
AcceptedPublic

Authored by MatzeB on Jun 1 2023, 6:11 PM.

Details

Summary

One problem that makes adding new tools to the test-suite complicated is the fact that timeit appends the exit code to the produced output file. So when using alternative tools we cannot simply redirect output to a file as that will miss the exit code.

The feature to expect non-null exit code is only used by a handful of benchmarks/tests and is not critical. This gets rid of the feature and the complexity and simply always expects an exit code of 0 in the future.

  • Changes some benchmark code to return a 0 exit code in the code paths used by our testing:
    • MultiSource/Applications/minisat/Main.cpp
    • MultiSource/Applications/spiff/spiff.c
    • MultiSource/Benchmarks/MiBench/office-ispell/ispell.c
    • MultiSource/Benchmarks/Prolangs-C/archie-client/archie.c
    • MultiSource/Benchmarks/Prolangs-C/football/stats.c
    • SingleSource/UnitTests/testcase-Expr-1.c
    • SingleSource/UnitTests/testcase-ExprConstant-1.c
  • Change test inputs to not end up in error state:
    • MultiSource/Benchmarks/Prolangs-C/gnugo/stdin
  • Adds a DO_NOT_RUN option and uses it for SingleSource/UnitTests/Matrix/AMXINT8 (instead of expecting an "illegal opcode" exit status)
  • Allows us to get rid of the "traditional_output" setting that was used to enable/disable the behavior of timeit to add the exit code.

Diff Detail

Repository
rT test-suite

Event Timeline

MatzeB created this revision.Jun 1 2023, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 6:11 PM
MatzeB requested review of this revision.Jun 1 2023, 6:11 PM
MatzeB added a comment.Jun 1 2023, 6:23 PM

This change enables adding support for perf stat and valgrind --tool=callgrind as additional benchmark runners (D151958 and D151959).

  • Changes to the makefile based build are done on a best-effort basis without testing.
  • I was unable to update reference output for big-endian tests that hash the outputs (assuming someone actually still runs the test-suite on a big-endian setup, that person would have to provide/update those).
MatzeB edited the summary of this revision. (Show Details)Jun 1 2023, 6:29 PM
MatzeB added reviewers: Meinersbur, fhahn, tarunprabhu.
MatzeB edited the summary of this revision. (Show Details)Jun 1 2023, 6:38 PM
MatzeB edited the summary of this revision. (Show Details)

I looked at the edits to the build files in the Fortran directory, and they look ok. I didn't go through the changes to every single reference output file, however.

I am not familiar with other parts of the test-suite, so I will defer to other reviewers.

You might consider adding @kiranchandramohan, @sscalpone and @klausler for another set of eyes on the changes in Fortran/fcvs21_95.

tra added a comment.Jun 2 2023, 9:55 AM

LGTM for CUDA test changes.

fhahn added a comment.Jun 2 2023, 12:33 PM

Sounds like a great idea to me, thanks for tackling this!

LGTM for AMXINT8 change. Thanks, @MatzeB.

ping, anyone wants to accept this as a whole?

MatzeB retitled this revision from RFC: Do not append exit status to reference_output files to Do not append exit status to reference_output files.Jun 29 2023, 11:26 AM
paquette accepted this revision.Jun 29 2023, 6:16 PM

Seems like we generally have consensus that this is a good idea, so LGTM?

This revision is now accepted and ready to land.Jun 29 2023, 6:16 PM