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

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/D16218#328615, @kristof.beyls wrote:

> 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.


Yep that is also the main motivation for me doing the cmake/lit work (the reusability of the benchmark framework).

> 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.


I've been thinking about this some more: I think the important thing here is that a user can easily reproduce the benchmark runs that lit did by himself. The user might want to run the commands in a debugger, modify them slightly for experimentation, etc. What this means for lit is that the only allowed way to modify the environment is through shell commands. A violation to this would be things like lit appending additional information to the program output file before doing verification, because that would prohibit a user from reproducing the benchmark run independently of lit. With that in mind:

- This patch is fine as all it does is add/modify the shell commands that get executed by lit. So LGTM with nitpick addressed.
- Most of what RunSafely is also modifying commandlines so it looks like a good candidate to get rewritten in python, however it currently has the nasty habbit of appending an "exit XX" line at the end of the programs output, however I think we can sneak this functionality into `timeit` and have the rest of RunSafely in lit.cfg.

> 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.


That's an interesting question, making parts of test-suite/lit.cfg available as modules in llvms lit. On top of attaching to the existing lit testsuite it would also help us from accidental API breaks which we had two times already where llvms lit was changed but the test-suite lit.cfg was not adapted.
On the other hand we have to convince people that it is worth keeping the stuff in llvms repository even though it is not directly used there.

> 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.


This is by accident, going for the pep8 standard makes sense. I'll try running the pep8 tool (or is there something better?) on it and fixing the issues.


================
Comment at: lit.cfg:180-189
@@ +179,12 @@
+        if litConfig.params.get('profile') == 'perf':
+            # wrap linux perf command to do profiling
+            profilefile = tmpBase + ".perf_data"
+            profilescript=[]
+            for line in runscript:
+                profilescript.append(
+                    ' '.join(
+                        ['perf record -e cycles,cache-misses,branch-misses -o',
+                          profilefile, line]))
+            profilescript = wrapScriptInRunSafely(profilescript,
+                                                  outfile=tmpBase+".perf.out")
+
----------------
Please move this into an own function.


http://reviews.llvm.org/D16218