In D103820#2803573, @RKSimon wrote:

@davezarzycki What's going on the way phab is reporting the path in Revision Contents pane? llvm/lib/Target/X86 -> i/llvm/lib/Target/X86 ?!?

I filed a bug about the IR not being canonicalized. Let's wait for that before proceeding on generating VPMOVQ2M:

In D103820#2803182, @craig.topper wrote:

In D103820#2803120, @davezarzycki wrote:

Actually, wait. Something weird is going on at the mid-level. These two functions should generate the same optimized IR, right?

typedef int V __attribute__((vector_size(64)));

V lt_zero_x_y(V mask, V x, V y) { return mask <  0 ? x : y; }
V ge_zero_y_x(V mask, V x, V y) { return mask >= 0 ? y : x; }

I'm not sure we canonicalize select operand order.

vpmovq2m only exists with avx512dq. Is this transform still useful with just avx512f?

Actually, wait. Something weird is going on at the mid-level. These two functions should generate the same optimized IR, right?

I'm surprised that this didn't break existing tests. Where is the right place to update the test suite?

https://bugs.llvm.org/show_bug.cgi?id=50516

LGTM but I don't feel ownership over this code.

Sorry for the noise. I reran the bisect. The test in question is still failing, but it turns out to be failing unreliably so git-blame is fairly useless without a reliable test.

I have an auto-bisecting multi-stage cron job that has identified this change as the source of a regression. Does the following seem plausible?

I've verified that this patch no longer causes my multi-stage cron job to regress. Thanks!

Actually, it is more complicated, and you're right that it also can involve overflow. I'm not sure if this is a trivial optimization when dependent comparisons are involved.

I can check it, but I'm a bit concerned about the comment and what it implies. The problem is NOT overflow, but that comparison operations need to be redistributed during this optimization (or dependent comparisons need to disable the optimization). So in pseudocode, the original code that miscompiled looks like this:

Thanks! Also, I feel fairly confident about this. My auto-bisecting cron job is really paranoid. For example, it always does a multi-stage test to verify that the commit *before* the commit blamed by git-bisect actually works. That's what I was waiting for. Also, I then verified by trying to move one commit ahead in the history to this commit to get an A/B test and I then confirmed that this is the regression. Thanks again for reverting this.

Verified. Second stage miscompile. If it matters (and it sometimes does), my first stage is built without asserts.

Lots of crashes like this:

FAIL: Clang-Unit :: Format/./FormatTests/FormatTest.FormatsCompactNamespaces (23265 of 76779)
******************** TEST 'Clang-Unit :: Format/./FormatTests/FormatTest.FormatsCompactNamespaces' FAILED ********************
Note: Google Test filter = FormatTest.FormatsCompactNamespaces
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.FormatsCompactNamespaces
FormatTests: /home/dave/ro_s/lp/clang/lib/Basic/SourceManager.cpp:865: clang::FileID clang::SourceManager::getFileIDLoaded(unsigned int) const: Assertion `0 && "Invalid SLocOffset or bad function choice"' failed.
 #0 0x0000000000604313 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x604313)
 #1 0x0000000000602962 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x602962)
 #2 0x0000000000604d8a SignalHandler(int) Signals.cpp:0:0
 #3 0x00007ffff7fa4a20 (/lib64/libpthread.so.0+0x13a20)
 #4 0x00007ffff7b392a2 raise (/lib64/libc.so.6+0x3d2a2)
 #5 0x00007ffff7b228a4 abort (/lib64/libc.so.6+0x268a4)
 #6 0x00007ffff7b22789 (/lib64/libc.so.6+0x26789)
 #7 0x00007ffff7b31a16 (/lib64/libc.so.6+0x35a16)
 #8 0x000000000064922c clang::SourceManager::getFileIDLoaded(unsigned int) const (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x64922c)
 #9 0x00000000003e734c clang::SourceManager::getDecomposedLoc(clang::SourceLocation) const CleanupTest.cpp:0:0
#10 0x000000000064b231 clang::SourceManager::isBeforeInTranslationUnit(clang::SourceLocation, clang::SourceLocation) const (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x64b231)
#11 0x000000000066c05e clang::format::AffectedRangeManager::computeAffectedLines(llvm::SmallVectorImpl<clang::format::AnnotatedLine*>&) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x66c05e)
#12 0x00000000006ae393 clang::format::UsingDeclarationsSorter::analyze(clang::format::TokenAnnotator&, llvm::SmallVectorImpl<clang::format::AnnotatedLine*>&, clang::format::FormatTokenLexer&) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x6ae393)
#13 0x0000000000690d1a clang::format::TokenAnalyzer::process() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x690d1a)
#14 0x0000000000665643 std::__1::__function::__func<clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*)::$_4, std::__1::allocator<clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*)::$_4>, std::__1::pair<clang::tooling::Replacements, unsigned int> (clang::format::Environment const&)>::operator()(clang::format::Environment const&) Format.cpp:0:0
#15 0x0000000000655d6e clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x655d6e)
#16 0x000000000065640f clang::format::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, llvm::StringRef, clang::format::FormattingAttemptStatus*) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x65640f)
#17 0x00000000003ef27f clang::format::(anonymous namespace)::FormatTest::format(llvm::StringRef, clang::format::FormatStyle const&, clang::format::(anonymous namespace)::FormatTest::StatusCheck) FormatTest.cpp:0:0
#18 0x0000000000407eed clang::format::(anonymous namespace)::FormatTest_FormatsCompactNamespaces_Test::TestBody() FormatTest.cpp:0:0
#19 0x000000000060cc53 testing::Test::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60cc53)
#20 0x000000000060dc16 testing::TestInfo::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60dc16)
#21 0x000000000060e470 testing::TestSuite::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60e470)
#22 0x000000000061bb23 testing::internal::UnitTestImpl::RunAllTests() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x61bb23)
#23 0x000000000061b59d testing::UnitTest::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x61b59d)
#24 0x000000000060555b main (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60555b)
#25 0x00007ffff7b23b75 __libc_start_main (/lib64/libc.so.6+0x27b75)
#26 0x00000000003ddd8e _start (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x3ddd8e)

Just a heads up, my auto-bisecting multi-stage cron job has identified this change as the source of a second stage regression in a bunch of clang unit tests. I'm about to verify by hand, but it'll take a while.

LGTM but I don't feel authoritative for the lit code.

Yes, Fedora 34 (x86-64).

I have an auto bisecting cron job that has twice identified this change as the source of a regression when building release (no asserts). Is the following expected or can we get a quick fix?

It's still failing but I can disable HSA on this machine for now. FYI --

FAILED: tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o
/p/tllvm/bin/clang++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/amdgpu-arch -I/home/dave/ro_s/lp/clang/tools/amdgpu-arch -I/home/dave/ro_s/lp/clang/include -Itools/clang/include -Iinclude -I/home/dave/ro_s/lp/llvm/include -Werror=switch -Wno-deprecated-copy -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O2 -DNDEBUG  -march=skylake -fno-vectorize -fno-slp-vectorize -fno-tree-slp-vectorize -fno-exceptions -fno-rtti -std=c++14 -MD -MT tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o -MF tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o.d -o tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o -c /home/dave/ro_s/lp/clang/tools/amdgpu-arch/AMDGPUArch.cpp
/home/dave/ro_s/lp/clang/tools/amdgpu-arch/AMDGPUArch.cpp:14:10: fatal error: 'hsa.h' file not found
#include <hsa.h>
         ^~~~~~~
1 error generated.

I removed all HSA/ROCM via the package manager and everything is fine now. Have you considered using __has_include() instead of CMake? It seems to be supported by all contemporary compilers, even MSVC.

In D99949#2709785, @JonChesterfield wrote:

It's not obvious to me why any of those stages would get a different result for the search for rocr. Do you do things with chroot/jails to ensure isolation for some of them?

In D99949#2709673, @JonChesterfield wrote:

That's interesting. The various permutations I have on disk are also all path/include/hsa/hsa.h. The existing in tree use of hsa.h is the amdgpu plugin, which uses #include "hsa.h" and #include <hsa.h>, which seems unlikely to be correct.

I'm going to patch this on the fly to hsa/hsa.h as that looks very likely to be the fix.

edit: or not, the include path cmake summons on my local system is -isystem $HOME/llvm-install/include/hsa, so hsa/hsa doesn't work.

Tiresome. Will revert this for now.

And if it helps:

In D99949#2709497, @JonChesterfield wrote:

In D99949#2709297, @davezarzycki wrote:

This change broke multi-stage builds (even if AMDGPU is disabled as a target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't find hsa.h. Is there a quick fix?

That should not be the case. The cmake for building AMDGPUArch.cpp is guarded by

find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm)
if (NOT ${hsa-runtime64_FOUND})
    return

The intent is for this to be built only when hsa-runtime64 is found on disk, at which point hsa.h is meant to be found within that package via cmake magic. Perhaps we need to explicitly specify the include directory, though we do not do so in the openmp plugin this was copied from, and I do not have hsa.h on any global search path.

Little harm will be done by reverting this (again...) and reapplying, but it would be really helpful if you can share some more information about the build config that is failing.

This change broke multi-stage builds (even if AMDGPU is disabled as a target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't find hsa.h. Is there a quick fix?

I'm the third person to notice the same problems, but this time on Fedora 34 (x86-64) so I reverted the change.

My mistake. The bisect tripped over a unreliable test. The real problem was 04733181b5136e2b3df2b37c6bdd9e25f8afecd0 and people are already piling onto that as being broken.

My automatically bisecting cron job identified this commit as the source of a regression on Fedora 34 (x86-64). It seems like a few dozen libc++ tests are now failing with errors like this:

In D98179#2689079, @lebedev.ri wrote:

In D98179#2689075, @mstorsjo wrote:

Something related to the time recording seems to fail intermittently on buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697

I'll cut to the fix here: @davezarzycki is there any reason why the timing data is stored in a CSV format, and not JSON?
The fix is to switch to latter.

Sorry, I got distracted. If it were up to me, I'd prefer tuning numbers that are, without checking, unambiguously better than the current code gen. In particular the code gen for KNL is awful. Furthermore, I think that fine tuning this further has diminishing returns for various reasons. Are people okay with that?

In D98884#2672170, @madhur13490 wrote:

These failures were not caught by LLVM bots which run on a patch. Can I do a trial run of the bots and run all tests that would run when the patch lands?

I have an auto-bisecting cron job that has identified this change as a second-stage regression on Fedora 34 (x86-64). Basically llvm-tblgen crashes reliably:

FAILED: lib/Target/X86/X86GenDAGISel.inc
cd /tmp/_update_lc/t && /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.  Program arguments: /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
#0 0x000000000050b153 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/bin/llvm-tblgen+0x50b153)
#1 0x00000000005094b2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/llvm-tblgen+0x5094b2)
#2 0x000000000050b4fa SignalHandler(int) Signals.cpp:0:0
#3 0x00007ffff7fa6a00 __restore_rt sigaction.c:0:0
#4 0x0000000000316a7d (/tmp/_update_lc/t/bin/llvm-tblgen+0x316a7d)

Good catch! Thanks

In D97913#2647721, @ldionne wrote:

In D97913#2637023, @davezarzycki wrote:

In D97913#2635064, @ldionne wrote:

Why do you use these settings?

-DLIBCXX_ENABLE_SHARED=FALSE
-DLIBCXXABI_ENABLE_SHARED=FALSE
-DLIBUNWIND_ENABLE_SHARED=FALSE

What is the intent of your build job?

I try to ensure that building/testing works with PIC disabled, which (of course) requires disabling shared libraries.

@davezarzycki The problem here is that this change enabled the libc++ and libc++abi tests in your job, which were not being run before. I think there might be issues with the configuration you're using and this change only makes those issues apparent.

In D95713#2647548, @teemperor wrote:

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

I've reverted this. If you need help debugging this, please let me know. Also, as a reminder, please see the attached build log from earlier in the conversation. Thanks!

An auto-bisecting cron job has identified this change as a regression on Fedora 33 (x86-64). Can we get a quick fix or revert this? Here is the build output:

Interesting. Thanks for looking into this and building on D98179. I'm sure that people will appreciate this change. :-)

Adding -stdlib=libstdc++ fixes it. Clang can default to libcxx so if the C++ standard library matters then the test needs to be explicit.

The very first RUN line seems to fail. Here is the output, sans FileCheck:

In rG243333ef3ec6c1e3910eb442177c2e2e927e6a87#984260, @MaskRay wrote:

This seems a misfire. bed9933a461e7b3d0c8c5a8fa770aa1b49802660 had fixed the failure.

In D98179#2639554, @nlopes wrote:

In D98179#2639491, @davezarzycki wrote:

In D98179#2639476, @nlopes wrote:

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

That still sounds incredibly brittle. If there is any variety in test machine performance, then any and all attempts at sorting should be futile because the underlying hardware will perturb different timeouts. Is this not your experience? How do you reconcile hardware performance and configuration details (like SMT) with timeout settings?

Of course it's brittle :) Changing from a time-based setting to a ticks-based system is ongoing work, such that resource exhaustion becomes deterministic.
Nevertheless, on a same machine, we don't see many test flips. It's quite stable most of the times (just one test flip once in a while).

In D98179#2639476, @nlopes wrote:

I'm talking about sorting just the summary of failed tests, not the whole output. We need the whole -vv output, but that can be out of order.

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

In D98179#2639483, @lebedev.ri wrote:

In D98179#2639476, @nlopes wrote:

I'm talking about sorting just the summary of failed tests, not the whole output. We need the whole -vv output, but that can be out of order.

Aha! +1 then, i think sorting summary should be both easy and nice to have.

In D98179#2638837, @nlopes wrote:

In D98179#2638801, @mehdi_amini wrote:

Can we revert to the previous behavior please? The current behavior is not user friendly. Thanks!

To clarify: you care about the order in the final summary, not the actual execution order, right? (the goal of this patch is the latter, if it changes the former this is just a side-effect I believe)

I don't love that the default test order was changed. For our use case that isn't good, but I can just delete the .lit_test_times.txt file and get the old fixed order. (the reason why having a fixed order is good is because of timeouts. Also, slow tests often consume more memory in our setting, so it's not good to run all the slow tests at the same time).

In D98179#2638882, @mehdi_amini wrote:

No, I'm running lit and dumping into a file. sorting is not as easy as piping through sort, as I'm running with -vv (required).

Since tests run in parallel, don't you already have some non-determinism in the -vv output? Isn't it printing as test finish? (just curious, not pushing back on anything here).

In D97913#2635064, @ldionne wrote:

Why do you use these settings?

-DLIBCXX_ENABLE_SHARED=FALSE
-DLIBCXXABI_ENABLE_SHARED=FALSE
-DLIBUNWIND_ENABLE_SHARED=FALSE

What is the intent of your build job?

I've run out of time for today, but for the record, I think we should use slash as the canonical separator for the timing data file. The actual separator doesn't matter because these strings are never used as paths, just as keys into a dictionary. (Therefore any character would work.) This would let us harmonize the Unix and Windows timing data files.

Ah yes, of course. I've marked that test as unavailable on Window for now: 61ca706461c5e1edc18526c9ddc3250fe074ed94

Try: 49d0e115d5df40aa89339f4ace7a8dee378c03bb

Said differently, the Unix side seems to always run the lit tests from a fresh/clean setup, even for incremental builds. If Windows tests lit differently then ya, things will blow up.

That implies a different testing environment. How different is the Windows testing environment compared to Unix? Do you specifically have a custom testing setup?

Removed the commented only-failures and fixed a nit before committing.

Might somebody be willing to sign off on this change (this week or next)? I'd like to cherry-pick it to Swift's LLVM branch. Thanks for all the feedback so far.

I fixed the call to split as requested. Being unfamiliar with python, I didn't know that it supported keyword arguments / parameter labels. Thanks.

I've made all of the requested changes to date. Two notes:

I'm downgrading libcxx as a reviewer to non-blocking because the latest patch no longer requires changes to their project.

I renamed the file variable as requested.

I believe I have addressed all of the feedback to date.

I believe I've addressed all of the feedback to date.

In D98179#2611438, @lebedev.ri wrote:

Nice!
I think this should also be used to improve the ETA time prediction.

Q: Where are these time files stored? I think it would not be good to store them next to the tests themselves.

Whoops. A couple regression were introduced when I tried to simplify this patch before posting it. Please don't try this yet.

For people following along, here is the replacement differential: https://reviews.llvm.org/D98179

From my build script:

In D97046#2609772, @jdenny wrote:

In D97046#2609763, @davezarzycki wrote:

I have a patch that implements the record-and-reorder feature. It turned out to be simpler than I feared. Now I just need to figure out how to write a test for it that makes sense and is reliable. For ninja check-llvm, the record-and-reorder mode completes the test suite 51% faster on my 48-core machine. I think it's time to abandon this change proposal.

Nice! Thanks for all your work.

If I understand how the record-and-reorder feature works, I assume you can test the record and reorder phases separately. The reorder phase should be easy to test with -j1. The record phase is probably harder to test because it depends on test times. You could at least check that it produces a sanely formatted record. I suppose it might be possible to also have one test sleep for a long time and then check that it is ordered before one that terminates immediately, but I'm not sure how brittle that will be on the bots, and it will slow down lit's test suite.

I have a patch that implements the record-and-reorder feature. It turned out to be simpler than I feared. Now I just need to figure out how to write a test for it that makes sense and is reliable. For ninja check-llvm, the record-and-reorder mode completes the test suite 51% faster on my 48-core machine. I think it's time to abandon this change proposal.

In D97046#2607752, @yln wrote:

@lebedev.ri
I don't think we need a RFC for this as long as nothing changes for people who don't use the early_tests or new, generalized ordering feature. We shouldn't make Dave pay for past debts. (early_tests was introduced 5 years ago!)

@davezarzycki
Thanks for explaining so patiently!

I now understand that there are two separate, but related features: record-and-reorder, and manually declared test phases. Let me summarize my understanding so you can check it.

record-and-reorder:
The sole goal here is to optimize execution time. lit can do this transparently, so maybe it would even make a good default so everyone can benefit. Making it the default is maybe something that benefit from an RFC so we can learn about people's concerns.
The one detail I want to push back on is that Dave argued that this would not apply to clean/full builds. Assume we store the order into a file. With enough motivation there is certainly a way to configure CI bots not to delete this file (or place it somewhere where it wouldn't be deleted).

manually-declared order:
Multiple goals: optimize execution time, execute high/low value tests first/last. Any other reasons to control order?
Although it is unfortunate that we have some overlap with --incremental here, I get that it doesn't accomplish all of these goals at the same time.

For me, the question is now whether the additional utility of "manually-declared order" justifies having it or if we should just got with "record-and-reorder". I think Dave is trying to make the argument that the answer here is "yes". I can see his arguments and would be happy to sign-off on an implementation, especially if the underlying machinery could be re-used to implement the other.

Design-wise I would prefer a solution that directly states test path prefixes, not "test phases". I think your sketch already did this, but let me restate:

// simply a list or dict of `path prefix -> priority`
config.test_order = [
'very/slow/test.c',
'directory/of/slow-tests/',
'<magic-value-for_all-other-tests>',  // name just for demonstration purposes ;)
'directory/of/low-value-tests/',
]

In lit configs, prepending to this list would mean "execute first", appending would mean "execute last".
And I imagine that "record-and-reorder" could be implemented (in a separate effort) on top of this by expanding <magic-value-for_all-other-tests> with the recorded order. (This would mean that the manually declared order is always respected.)

Let me know what you think