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

Fri Mar 5 09:59:07 PST 2021

mehdi_amini added inline comments.

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

----------------
davezarzycki wrote:
> 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.
It is very possible that my "fear" is over-exaggerated and only ~5 test would be listed in the "phase", I won't really object to you patch here, feel free to proceed (assuming no one else has strong objections).
If you can name it with "hint" in the name somehow that'd be better indeed!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97046