[PATCH] D76178: [lit] Recursively apply substitutions

Mon Mar 16 07:35:35 PDT 2020

ldionne added a comment.

In D76178#1922945 <https://reviews.llvm.org/D76178#1922945>, @dexonsmith wrote:

> This looks really cool.  Can you add tests for this feature, somewhere inside `llvm/utils/lit/tests`?

Yes, I will, I just wanted to gauge interest first!

> It would also be good to confirm that this isn't expensive in the normal case (one-level substitutions). Have you timed running the libc++ tests with and without this patch?

I can't see any noticeable difference running the Clang or the libc++ tests -- all differences appear to be noise:

  # Clang with patch
  real    2m43.026s
  user    18m27.649s
  sys 16m20.605s

  # Libc++ with patch
  real    18m46.297s
  user    35m22.514s
  sys 5m27.994s

  # Clang without patch
  real    3m6.585s
  user    19m26.953s
  sys 15m50.688s

  # Libc++ without patch
  real    18m47.104s
  user    35m23.633s
  sys 5m29.889s

In D76178#1922990 <https://reviews.llvm.org/D76178#1922990>, @delcypher wrote:

> While I can see the appeal of this I need convincing that this actually needed. I see too many downsides
>
> - Possibility of non-termination being introduced that we'll have to check for which is a cost that will have to be paid every time we execute a test.

I agree we should check for this, however it's not clear to me this is going to make a noticeable perf difference in practice. The tests we run involve executing processes and doing things that are a lot more expensive than the simple lit substitution, even if we check for  non-terminating substitutions. I think it should be possible to make it fairly quick in the usual case where full substitution happens after exactly one substitution.

> - Debugging lit substitutions becomes really difficult. Right now I can add a few print statements in the lit.site.cfg files to print out the current substitutions (or ones that interest me) to see what they expand to. With this change that doesn't work anymore because the full expansion happens much later.

I'm not sure I understand. Full expansion after this patch happens at the same point where it does before the patch. The only difference is that when you print substitutions in the `lit.site.cfg`, you must mentally substitute anything that would normally get re-substituted with this patch. However, if you don't have substitutions that expand into other substitutions, this is moot -- the behavior is the same as today.

> The way most test suites handle achieving your end goal is to just have variables that represent the different pieces of a command line and then define the substitution appropriately. E.g.
> 
> [...]
> 
> While not quite as elegant as what this patch proposes it is so much easier to debug. Are you trying to do something that is difficult to express in the `lit.site.cfg` files?

Yes, that's indeed what libc++ does. It actually ends up being quite contrived in libc++, and we have a couple substitutions that would benefit from this.

The larger picture of why I got to making this change is that I'm trying to rewrite the libc++/lit integration entirely to make it simpler and more flexible. The way I've done that so far is by defining the libc++ test format right on top of `executeShTest`, where my test format only expects a few substitutions to be provided in the config object (`%cxx`, `%compile_flags`, and a few others), and everything else is defined on top of those substitutions. For example, a `.pass.cpp` test is one with a script of two steps:

- `%cxx %compile_flags %link_flags -o %t.exe %s`
- `%exec %t.exe`

The benefit of approaching it this way is that it's as flexible as what we have today, however it's incredibly easy to extend by tweaking what substitutions you provide in the `lit.cfg`. For example, if you want a custom executor, just define the `%exec` substitution to run your custom script, and you don't need to write a hundred lines of Python code that calls `subprocess(...)`. Where recursively expanding substitutions come into the picture is that I define `%build` on top of `%cxx` (and others), and I don't have access to the actual thing that `%cxx` will be substituted for at the point where I define `%build`. In other words, I'm parameterizing substitutions on top of a set of base substitutions. I've found this to be very powerful and flexible so far.

@delcypher 
Is there an interest for moving forward with this change if we check for non-termination without incurring unreasonable cost, or are you not convinced by my use case?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76178