Page MenuHomePhabricator

[Polly] Build and run isl_test as part of check-polly
ClosedPublic

Authored by grosser on Oct 1 2016, 1:09 AM.

Details

Summary

Running isl tests is important to gain confidence that the isl build we created
works as expected. Besides the actual isl tests, there are also isl AST generation tests
shipped with isl. This change only adds support for the isl unit tests. AST generation test
support is left for a later commit.

There is a choice to run tests directly through the build system or in the context of lit. We choose to run tests as part of lit to as this allows us to easily set environment variables, print output only on error and generally run the tests directly from the lit command.

Diff Detail

Repository
rL LLVM

Event Timeline

grosser updated this revision to Diff 73189.Oct 1 2016, 1:09 AM
grosser retitled this revision from to [Polly] Build and run isl_test as part of check-polly.
grosser updated this object.
grosser added a reviewer: Meinersbur.
grosser added subscribers: llvm-commits, pollydev.
grosser added a subscriber: brad.king.

@brad.king (or others), do you have any idea how I can make a add_custom_command be silent in non-verbose mode (e.g., runwith 'ninja check-polly-isl', not with 'ninja -v check-polly-isl'). Me piping to /dev/null/ seems both hackish and not very platform independent?

brad.king edited edge metadata.Oct 3 2016, 5:05 AM

make a add_custom_command be silent in non-verbose mode

I don't think Ninja has an option for this. If the command produces output Ninja will show it. Ninja encourages the convention of no output on success.

Meinersbur edited edge metadata.Oct 3 2016, 5:28 AM

I'd prefer to call it from the lit suite, like all the executables in unittestts/, for consistency. It would automatically count it as passed test and list it if it fails. Should be possible by editing lit.cfg.

lib/External/CMakeLists.txt
298 ↗(On Diff #73189)

This is no proper output. For commands without any output file, use

add_custom_target(
  COMMAND ...
299 ↗(On Diff #73189)

Cmake allows to change environment variables like this:

COMMAND ${CMAKE_COMMAND} -E env srcdir="${ISL_SOURCE_DIR}" polly-isl-test
lib/External/isl/isl_test.c
6698–6702 ↗(On Diff #73189)

Sure you want to change files in the External/isl directory? Previously, this was a no-go for you.

I'd prefer to call it from the lit suite, like all the executables in unittestts/, for consistency. It would automatically count it as passed test and list it if it fails. Should be possible by editing lit.cfg.

Meinersbur added inline comments.Oct 3 2016, 7:45 AM
lib/External/CMakeLists.txt
299 ↗(On Diff #73189)

> /dev/null is platform-specific (not working on Windows). I also expected the redirection operator > to be escaped by VERBATIM st. is passes as command-line parameter, but it does not happen.

lib/External/isl/isl_test.c
6716 ↗(On Diff #73189)

Indention

grosser updated this revision to Diff 73306.Oct 3 2016, 11:04 AM
grosser edited edge metadata.

@Meinersbur, I like the idea of running isl_test through lit and tried to
implement this.

I originally had hoped that I would manage to reuse one of the existing lit.cfg
instances, but did not find a good way to do so. I cannot use the existing unit
test lit instance, as the isl unit tests are not gtest based. I also cannot use
the normal polly tests, as this would mean we would run the isl unit tests with
each call to 'check-polly-tests', which would add 6 seconds of almost never
failing unit tests. I do not think this is what we want either.

Instead, I decided to create a new lit root which looks for .sh files. One of
these files runs ./isl_codegen. Even though this approach works, I am a little
unhappy with the verbose copy of lit.cfg and also just calling isl_codegen from
a shell file looks a little hacky. However, this approach seems to do exactly
what we want without the need for any isl modifications. Does this
implementation correspond with what you had in mind? Any ideas how this can
be further improved?

TODO: I still need to test this with out-of-tree builds of Polly.

@brad.king: thanks for clarifying.

@Meinersbur: Using lit is an interesting idea, I implemented this in the most recent patch.

I need to cross-check this patch (especially the Polly out-of-tree builds), but wanted to see if this new approach goes into the direction you thought of.

lib/External/CMakeLists.txt
299 ↗(On Diff #73189)

Cmake allows to change environment variables like this:

COMMAND ${CMAKE_COMMAND} -E env srcdir="${ISL_SOURCE_DIR}" polly-isl-test

Does this work on Windows?

"> /dev/null" is platform-specific (not working on Windows). I also expected the redirection operator > to be escaped by VERBATIM st. is passes as command-line parameter, but it does not happen.

I know. This is why I wrote in the commit message that I am looking for better solutions. Your proposal to use 'lit' addressed this issue, as lit does not reprint the output of individual test cases as long as they do not fail.

lib/External/isl/isl_test.c
6698–6702 ↗(On Diff #73189)

I certainly would upstream such changes BEFORE I commit this patch to Polly. However, before I start a discussion on the isl mailing list, I wanted to see if the approach I propose works well or if the Polly community has better ideas. As it seems easy to set environment variables from lit, this change does not seem to be needed any more. Thanks for pointing out the idea of using 'lit'.

Thanks for the lit implementation!

I tried and got:

$ ninja check-polly-isl
[1/1] Running isl unit tests only
/root/build/llvm/release/tools/polly/test/../test
FAIL: Polly - isl unit tests :: isl_test.sh (1 of 1)
******************** TEST 'Polly - isl unit tests :: isl_test.sh' FAILED ********************
Script:
--
isl_test
--
Exit Code: 127

Command Output (stderr):
--
/root/build/llvm/release/tools/polly/test/Output/isl_test.sh.script: line 1: isl_test: command not found

--

********************
Testing Time: 1.17s
********************
Failing Tests (1):
    Polly - isl unit tests :: isl_test.sh

  Unexpected Failures: 1
FAILED: cd /root/build/llvm/release/tools/polly/test && /usr/bin/python2.7 /root/src/llvm/utils/lit/lit.py -sv --param polly_site_config=/root/build/llvm/release/tools/polly/test/UnitIsl/lit.site.cfg /root/build/llvm/release/tools/polly/test/UnitIsl
ninja: build stopped: subcommand failed.

It also runs with ninja polly-check-tests, presumbly because lit searches for lit.site.cfg's in all subdirectories. But, if you allow the question, if you don't want to run this by the default ninja check-polly, why implementing this in the first place?

test/CMakeLists.txt
77 ↗(On Diff #73306)

Why is this line duplicated?

test/UnitIsl/isl_test.sh
1 ↗(On Diff #73306)

Wouldn't the unittests folder be better suited?

test/UnitIsl/lit.cfg
109–111 ↗(On Diff #73306)

Given that PollyISL doesn't need anything from LLVM (here: opt), can we remove a lot of such lines?

Meinersbur added inline comments.Oct 4 2016, 6:07 AM
test/UnitIsl/isl_test.sh
1 ↗(On Diff #73306)

Please ignore this, I wasn't able to remove this line in Phabricator. It is in tests as all lit.cfg in any LLVM project is in the test subdirectory.

grosser updated this revision to Diff 73470.Oct 4 2016, 6:36 AM

Addres most of Michael's comments

grosser marked 3 inline comments as done.Oct 4 2016, 6:45 AM

Thanks for the lit implementation!

I tried and got:


Testing Time: 1.17s


Failing Tests (1):

  Polly - isl unit tests :: isl_test.sh

Unexpected Failures: 1

FAILED: cd /root/build/llvm/release/tools/polly/test && /usr/bin/python2.7 /root/src/llvm/utils/lit/lit.py -sv --param polly_site_config=/root/build/llvm/release/tools/polly/test/UnitIsl/lit.site.cfg /root/build/llvm/release/tools/polly/test/UnitIsl
ninja: build stopped: subcommand failed.

Interesting. It works for me. In which directory is the isl_codegen binary generated for you, if at all. To which value is the PATH variable set? (Does setting PATH even work on windows?)

It also runs with ninja polly-check-tests, presumbly because lit searches for lit.site.cfg's in all subdirectories. But, if you allow the question, if you don't want to run this by the default ninja check-polly, why implementing this in the first place?

Ideally, I wanted this to run as part of polly-check and polly-check-isl, but not polly-check-tests. The reason is that I thought you introduced polly-check-test to have an option to only run .ll tests, but no
unit tests. However, if polly-check-tests anyhow searches recursively, this is probably difficult to achieve.

Best,
Tobias

test/CMakeLists.txt
77 ↗(On Diff #73306)

I dropped it.

The original intention was to include the UnitIsl/ folder explicitly, but I forgot to rename /Unit to /UnitIsl. However, it seems to work even without this line.

test/UnitIsl/lit.cfg
109–111 ↗(On Diff #73306)

Yes.

Interesting. It works for me. In which directory is the isl_codegen binary generated for you, if at all. To which value is the PATH variable set? (Does setting PATH even work on windows?)

This was Linux (WSL)

isl_codegen is for a later commit as described in the summary?

polly-isl-test is in bin/polly-isl-test

test/UnitIsl/isl_test.sh
1 ↗(On Diff #73470)

Should this be polly-isl-test?

grosser updated this revision to Diff 73481.Oct 4 2016, 8:01 AM
grosser marked an inline comment as done.

Rename isl_test to polly-isl-test

It seems I had an old leftover isl_test binary in my build directory that
made the old code work just on my system.

grosser updated this object.Oct 4 2016, 8:05 AM

Interesting. It works for me. In which directory is the isl_codegen binary generated for you, if at all. To which value is the PATH variable set? (Does setting PATH even work on windows?)

This was Linux (WSL)

isl_codegen is for a later commit as described in the summary?

I meant isl_test.

polly-isl-test is in bin/polly-isl-test

A leftover isl_test from an older build made the tests work for me. I addressed this in the latest version.

Meinersbur accepted this revision.Oct 4 2016, 10:56 AM
Meinersbur edited edge metadata.

Works for me now: Windows, Linux, BUILD_SHARED=ON

This revision is now accepted and ready to land.Oct 4 2016, 10:56 AM
This revision was automatically updated to reflect the committed changes.