[PATCH] D16218: [test-suite] Add --param=profile=perf option to lit.

Kristof Beyls via llvm-commits llvm-commits at lists.llvm.org
Sat Jan 16 01:52:27 PST 2016


kristof.beyls added a comment.

In http://reviews.llvm.org/D16218#328281, @MatzeB wrote:

> Very Interesting!
>
> As for this patch I think you should put the perf wrapping code into an own function as well.
>
> Though seeing this I think we should have a discussion about the role of lit in the testsuite. lit is the obvious choice for scheduling tests and is nice as a commandline user interface, I also think we have managed to capture the essence of how to run a test into the .test files. However I think we need a discussion of where to put the logic for various variants of running tests. Right now this logic is mostly in RunSafely.sh (it collects results, sets up timeit, ssh to a device, ...) in that logic your change should be in RunSafely.sh as well. On the other hand I must admit RunSafely.sh is a horrible script, and we might be better off to rewrite the functionality in python and import that in lit.cfg...
>
> I am not against accepting your changes if you need them right now, but we definitely should start the discussion on how to organize the code/logic/scripts long term.


Many thanks for your thoughts!

I don't need these changes urgently - I created this patch to:

- give me an opportunity to learn a bit more about the details of the cmake/lit-ified test-suite design.
- add a feature I want to just the cmake/lit test-suite system to give me a stronger motivation to help progress it's development faster.
- investigate how to implement a first step towards having annotated assembly output in lnt's webui, highlighting the key reasons why performance changed after a given commit - see last part of my 2015 dev meeting presentation.
- verify if the cmake/lit-based way to drive the test-suite is indeed better by testing if I only had to change common code to make this feature work for both the test-suite and external benchmark suites. I have to confess I haven't tested this yet. For me, this is one of the biggest potential advantages of cmake/lit in driving the test-suite: being able to plug in other benchmark suites under External and reuse all benchmark framework features across all suites plugged in, without needing to make any adaptations to the plugged in suites to do so.

I don't have a strong preference over how the responsibilities of aspects of test execution are divided over different scripts, but I do agree that we probably should just rewrite RunSafely.sh in python. It probably should also get a different name, as it currently already doesn't cover just "running a test safely" anymore, but has more features.

Would it be a crazy idea to integrate RunSafely.sh functionality into LLVM's lit, as a separate component?
One advantage of that would be that we could more easily unit/regression test its functionality, as the LLVM lit suite already has regression tests, I think. I missed being able to add a regression test while creating this patch.

Another observation I had is that currently the lit.cfg file violates PEP8 in quite a few places.
In LNT, we try to stick to PEP8. I think we should do in lit.cfg too.


http://reviews.llvm.org/D16218





More information about the llvm-commits mailing list