[PATCH] D34362: [LNT] Support for different DataSet usage in Polybench for "lnt runtest nt"

[Utpal Bora via Phabricator]

cs14mtech11017 added reviewers: Meinersbur, grosser.
cs14mtech11017 added a subscriber: pollydev.

Repository:
rL LLVM

https://reviews.llvm.org/D34362

[Michael Kruse via Phabricator]

Meinersbur resigned from this revision.
Meinersbur added a comment.

I think `nt.py` is deprecated, and meant to be replaced by `test_suite.py`.

Maybe someone who already contributed to lnt should be necessary for acceptance.

[Kristof Beyls via Phabricator]

kristof.beyls added reviewers: MatzeB, cmatthews.
kristof.beyls added a comment.

In https://reviews.llvm.org/D34362#788285, @Meinersbur wrote:

> I think `nt.py` is deprecated, and meant to be replaced by `test_suite.py`.
>
> Maybe someone who already contributed to lnt should be necessary for acceptance.

A few of the most active developers on test-suite and lnt at least think it's better to use the (new) CMake+lit-based system to drive the test-suite rather than the (old) Makefile-based system. 'nt.py' in LNT is a layer around the Makefile-based system, "test_suite.py" is a layer around the CMake+lit-based system. I guess you could say that makes nt.py "deprecated".

Anyway, on the patch itself here: I don't really like adding ever more separate command line options for specifying different data sizes available in a test suite. Wouldn't it be better to introduce a command line argument spelled e.g. "--datasize=mini|small|standard|large|extra_large"? We could then also hopefully deprecate "--small", "--large", "--spec-with-ref", etc., making them aliases for the new command line option for backwards compatibility. Note that I haven't thought about what the best name would be for this option. Maybe "inputsize" is more in line with common terminology across benchmarks? Although it may be hard to guess from the "inputsize" exactly what it means without further explanation either...

Also, please in the future upload diffs with more context, see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

[Tobias Grosser via Phabricator]

grosser added a comment.

Thanks Kristof for jumping in. I think your comments are clearly pointing out the right direction. @cs14mtech11017 , I suggest you follow these suggestions.

[Matthias Braun via Phabricator]

MatzeB added a comment.

- Indeed, active test-suite development is done in the cmake build (test_suite.py in lnt). You may still change nt.py but landing features exclusively there would be bad.
- It seems the llvm test-suite today only checks for `SMALL_PROBLEM_SIZE`, occasionally `LARGE_PROBLEM_SIZE` and for SPEC you can choose the size manually. Do you plan to change the test-suite for the new sizes? We should probably do further discussions there.
- Do you plan to set any rules/expectations on those new sizes? (Something like "mini" finished in half the time than "small")
- Out of interest: Why do you need finer control over the problem sizes?

[Chris Matthews via Phabricator]

cmatthews requested changes to this revision.
cmatthews added a comment.
This revision now requires changes to proceed.

It makes more sense to me to be using the --make-param flag to pass a test specific configuration options. If you want to add all these size classes, all the tests should support them, or have them mapped back to nearest size the tests can handle.

[Utpal Bora via Phabricator]

cs14mtech11017 added a comment.

In https://reviews.llvm.org/D34362#788285, @Meinersbur wrote:

> I think `nt.py` is deprecated, and meant to be replaced by `test_suite.py`.
>
> Maybe someone who already contributed to lnt should be necessary for acceptance.

Ok. I will look into `test_suite.py` and update the patch.

[Utpal Bora via Phabricator]

cs14mtech11017 added a comment.

In https://reviews.llvm.org/D34362#788827, @kristof.beyls wrote:

> In https://reviews.llvm.org/D34362#788285, @Meinersbur wrote:
>
> > I think `nt.py` is deprecated, and meant to be replaced by `test_suite.py`.
> >
> > Maybe someone who already contributed to lnt should be necessary for acceptance.
>
>

> A few of the most active developers on test-suite and lnt at least think it's better to use the (new) CMake+lit-based system to drive the test-suite rather than the (old) Makefile-based system. 'nt.py' in LNT is a layer around the Makefile-based system, "test_suite.py" is a layer around the CMake+lit-based system. I guess you could say that makes nt.py "deprecated".
>
> Anyway, on the patch itself here: I don't really like adding ever more separate command line options for specifying different data sizes available in a test suite. Wouldn't it be better to introduce a command line argument spelled e.g. "--datasize=mini|small|standard|large|extra_large"? We could then also hopefully deprecate "--small", "--large", "--spec-with-ref", etc., making them aliases for the new command line option for backwards compatibility. Note that I haven't thought about what the best name would be for this option. Maybe "inputsize" is more in line with common terminology across benchmarks? Although it may be hard to guess from the "inputsize" exactly what it means without further explanation either...

I agree with you. I was planning to add such a flag for all the sizes "--datasize=mini|small|standard|large|extra_large". Did not do it for compatibility. Thanks for the suggestion about aliasing for backward compatibility. If we agree on adding such an option, I think "--testdatasize" will be appropriate. And I would argue about using a common flag like this one for all for all data sizes instead of using "--make-param" flag as pointed out by @cmatthews.

> Also, please in the future upload diffs with more context, see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

Ohh I missed that.

[Utpal Bora via Phabricator]

cs14mtech11017 added a comment.

In https://reviews.llvm.org/D34362#789152, @MatzeB wrote:

> - Indeed, active test-suite development is done in the cmake build (test_suite.py in lnt). You may still change nt.py but landing features exclusively there would be bad.

Thanks for letting me know. Will update for both.

> - It seems the llvm test-suite today only checks for `SMALL_PROBLEM_SIZE`, occasionally `LARGE_PROBLEM_SIZE` and for SPEC you can choose the size manually. Do you plan to change the test-suite for the new sizes? We should probably do further discussions there.

I added these options only for Polybench. Have not inspected the other benchmarks. I would like to discuss how we can map these sizes for other benchmarks? One we agree upon something, I can make the changes for the test-suite.

> - Do you plan to set any rules/expectations on those new sizes? (Something like "mini" finished in half the time than "small")

I have not thought about it yet.

> - Out of interest: Why do you need finer control over the problem sizes?

I am working with Polybench, which supports multiple input dataset sizes. I did these changes for my experiments and thought it would be better to upload the changes for the community.

[Utpal Bora via Phabricator]

cs14mtech11017 added a comment.

In https://reviews.llvm.org/D34362#789280, @cmatthews wrote:

> It makes more sense to me to be using the --make-param flag to pass a test specific configuration options. If you want to add all these size classes, all the tests should support them, or have them mapped back to nearest size the tests can handle.

I feel having a common flag for test data sizes is better than having to pass them as test specific flag. As you suggested, we can map them back to nearest data sizes for the other tests. I was working with Polybench, so did not look into other tests. Will proceed with it if everyone agrees on adding a flag like "--testdatasizes=mini|small|medium|large|extra_large" and aliasing --small and --large for backward compatibility.

[Chris Matthews]

That sounds good to me, if the test-suite gurus agree.

Do we submit dataset size to the server? We should make sure we do, so we don’t end up trying to compare a mini and a small by accident.

[Matthias Braun via Phabricator]

MatzeB added a comment.

In https://reviews.llvm.org/D34362#790215, @cs14mtech11017 wrote:

> In https://reviews.llvm.org/D34362#789280, @cmatthews wrote:
>
> > It makes more sense to me to be using the --make-param flag to pass a test specific configuration options. If you want to add all these size classes, all the tests should support them, or have them mapped back to nearest size the tests can handle.
>
>
> I feel having a common flag for test data sizes is better than having to pass them as test specific flag. As you suggested, we can map them back to nearest data sizes for the other tests. I was working with Polybench, so did not look into other tests. Will proceed with it if everyone agrees on adding a flag like "--testdatasizes=mini|small|medium|large|extra_large" and aliasing --small and --large for backward compatibility.

Look at SPEC for example: It has 3 different sizes: test, train and ref. You can describe them roughly as:

- test: "As fast as posisble, not useful as a benchmark but they touch enough code paths so that they can give you a quick way to test for correctness"
- train: "Somewhat realistic but smaller inputs, useful to produce data for PGO optimizations. The important aspect here is that the data is different enough from ref, so we don't overfit the code because training and reference data were the same"
- ref: "Larger inputs running for a longer time producing stable numbers".

So there is some semantics and intended uses here that is captured fine with "test", "train" and "ref". Mapping this to some generic terms like "small", "medium", "large" that just map to sizes would be a loss IMO.

[Kristof Beyls via Phabricator]

kristof.beyls added a comment.

Right, that's a fair point.

FWIW, "lnt runtest test-suite" doesn't have --small or --large or --spec-with-ref command line options; but it does have a --test-size option, seemingly with the same intention as the "--testdatasizes" command line option under discussion here.
That being said, that "--test-size" option currently doesn't seem to actually be doing anything...

I can see that in different circumstances, you'd either want the generic flag vs the SPEC-specific flags. For example:

- Assuming you want to run as many of the programs in the test-suite (with externals plugged in) as possible, you'd probably want to use the generic flag that maps to "small", "medium", "large" to control for how long the benchmark runs. Or how much memory it could end up using. Or maybe some other aspect of program scalability. I don't think I've seen small vs large defined anywhere particularly well...
- If you're only going to run the SPEC benchmarks, you'd probably want to be able to state to run "test", "train" or "ref", rather than "small", etc.
- Of course if you want to retain the original terms and meanings for data size used during benchmarking for SPEC, you can also argue you could want that for other benchmarks like Polybench.

So, in summary, I'm not sure what the conclusion here should be.
If we had a reasonable definition of what "small" or "large" meant, or intended to mean, that would make this decision easier.
For now, I think it's fine for --small and --large to map to traditional "small" and "large" semantics for the test-suite.
Maybe we should refrain from introducing further command line options in LNT for all possible benchmark-specific sizes and indeed set them using --cmake-define/--make-param, to avoid a further explosion of lnt runtest command line options?

[Kadir SELÇUK via Phabricator]

post.kadirselcuk requested changes to this revision.
Herald added a subscriber: dkolesnichenko.

Repository:
rL LLVM

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

https://reviews.llvm.org/D34362

[Kadir SELÇUK via Phabricator]

post.kadirselcuk commandeered this revision.
post.kadirselcuk edited reviewers, added: cs14mtech11017; removed: post.kadirselcuk.

[Kadir SELÇUK via Phabricator]

post.kadirselcuk changed the repository for this revision from rL LLVM to rZORG LLVM Github Zorg.
post.kadirselcuk added projects: 3.6-release, 3.7.1-merged, AMDGPU, Backend, C++ modules in LLDB, clang, clang-c, clang-modules, clang-tools-extra, commits-test, clang-format, JuliaLang, libc++, libc++abi, libc-project.
Herald added a reviewer: gkistanova.

Repository:
rZORG LLVM Github Zorg

[Michael Kruse via Phabricator]

Meinersbur added a comment.

What happened?

[MyDeveloperDay via Phabricator]

MyDeveloperDay removed a project: clang-format.