[PATCH] D97046: [lit] Generalize `early_tests`

Fri Mar 5 04:12:48 PST 2021

davezarzycki added a comment.
Herald added a subscriber: dcaballe.

@mehdi_amini asked "how it will be used" in the code review comments of this change but I want to answer that question in the main thread of this change proposal:

As I've mentioned a few times in this change proposal, the goal I have is to modify Swift to have five or so of the slowest tests listed and push the fast/low-reward "compiler crasher" tests to the end. My goal is twofold: low maintenance and zero changes to infrastructure/workflow (i.e. bots / CI / CMake files / people's build environments). When I look at LLVM proper, I only see only one big outlier test that ought to be pushed ahead of the rest for a decent performance win. I haven't checked clang, but I'd wager that <5 need to be listed for a decent performance win.

The way I look at the proposed `start_phases` feature is that it's a hinting mechanism, no more, no less. People aren't obligated to use it, and the hints are just about a better user experience, not correctness. If we need to put the word "hint" into the docs or even config variable, then I'd support that.

Finally, if people still have a strong aversion to this hinting based approach, I can look into the record-and-reorder approach. It's less than ideal for some scenarios that I care about but I can live with it. And unless I'm missing something, I don't see any precedent for lit recording anything to the test output directory.

================
Comment at: mlir/test/Unit/lit.cfg.py:17
+# start_phase; Request to start this suite early.
+config.start_phase = -1

----------------
mehdi_amini wrote:
> davezarzycki wrote:
> > mehdi_amini wrote:
> > > What is the motivation for MLIR Unittests to start early?
> > > I'd expect this to be at least documented. The comment here is just repeating *what* the code is doing instead of *why* we're doing this.
> > I don't know. Presumably some of them are slow. If you're really curious, then I'd recommend digging through the SCM history to see why the original `is_early` flag was added.
> This was just copied from LLVM as-is.
> 
> This kind of goes into my point about maintainability: the `is_early` was considered "hacky" (per author's words: https://reviews.llvm.org/D18089 ) but was coarse grain and minimalistic.  I'm not sure that the finer grain control introduced here goes in the same spirit.
> 
> So to make it clear: I don't think this feature is super intrusive to lit and the code is fine. It is more the "how it will be used in the repo" that makes me pause here.
Maybe we should just remove/deprecate the "is_early" hack then.

As to "how it will be used", I'll answer that in the main review feedback thread.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97046/new/

https://reviews.llvm.org/D97046