This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Refactor tests to generate binaries and profiles on the fly.
ClosedPublic

Authored by snehasish on Feb 28 2023, 5:43 PM.

Details

Summary

This change replaces the binary profiles and executables used for
testing the memprof profile reader with tests where the profiles are
generated on the fly. This reduces toil when the profile version
changes. The tests are moved from tools/llvm-profdata to
compiler-rt/test/memprof due to the following reasons:

  1. Adding dependency on memprof lit.cfg.py for llvm-profdata is preferable to adding a dependency on compiler-rt for llvm/test.
  2. All the tests can now be run with ninja check-memprof.

Diff Detail

Event Timeline

snehasish created this revision.Feb 28 2023, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 5:43 PM
Herald added a subscriber: Enna1. · View Herald Transcript
snehasish requested review of this revision.Feb 28 2023, 5:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 28 2023, 5:43 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
snehasish updated this revision to Diff 501349.Feb 28 2023, 5:47 PM

Cleanup some debugging code in lit.cfg.py

tejohnson accepted this revision.Mar 3 2023, 1:38 PM

lgtm - thanks! One small suggestion on the tests below.

compiler-rt/test/memprof/TestCases/memprof_basic.c
16

I believe it is typical to put the RUN lines at the top above the source code, so it might be good to do that in these tests as well.

This revision is now accepted and ready to land.Mar 3 2023, 1:38 PM
snehasish updated this revision to Diff 502262.Mar 3 2023, 2:29 PM

Move run lines and some other cleanup.

snehasish updated this revision to Diff 502267.Mar 3 2023, 2:46 PM

clang-format changes.

This revision was landed with ongoing or failed builds.Mar 6 2023, 1:24 PM
This revision was automatically updated to reflect the committed changes.

FWIW, there's a drawback here: Now llvm-profdata is missing test coverage in the llvm subproject. Someone making changes would reasonably expect that "check-llvm" would be adequate to test their changes. So we do try pretty hard to keep tests for code in the same subproject as the code itself. (but yes, also the llvm subproject can't/shouldn't depend on compiler-rt)

& as for version toil - presumably the existing/previous tests for the old version should remain to keep testing that functions correctly.

Perhaps a script to easily update things for new versions would be adequate?

dyung added a subscriber: dyung.Mar 6 2023, 3:22 PM

@snehasish 3 of the tests you moved/added in this change are failing on a build bot, can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/247/builds/2185

Perhaps a script to easily update things for new versions would be adequate?

Took a stab at it in D145644, PTAL thanks!

llvm/test/tools/llvm-profdata/memprof-inline.test