Page MenuHomePhabricator

test-suite: disable tests that fail when FP optimization is in effect, add support for per-ISA reference outputs
AbandonedPublic

Authored by Abe on Oct 13 2016, 1:27 PM.

Details

Summary

Note: the "SingleSource/Benchmarks/Polybench/stencils/adi" subtest varies not only across {-Ofast vs. -O2 or -O3 both withOUT -ffast-math} but also across {AMD64 vs. AArch64}.

Tested to pass on both AArch64 and AMD64 with both "-O3" and "-O3 -ffp-contract=fast", on top of:

commit d471bba591ad51ecf4180cebda63e486d2fa877c
Author: Sunil Srivastava <sunil_srivastava@playstation.sony.com>
Date:   Mon Oct 17 04:32:15 2016 +0000

[Abe is intentionally leaving the Polybench stuff for Sebastian since he has already started fixing it]

[Abe is intentionally leaving the "-Ofast"-compatibility work for a different patch, at MB`s request.]

Diff Detail

Event Timeline

Abe updated this revision to Diff 74567.Oct 13 2016, 1:27 PM
Abe retitled this revision from to test-suite: try to force many FP-dependent tests to compile with unaggressive FP optimization, add support for per-ISA reference outputs.
Abe updated this object.
Abe added a reviewer: sebpop.Oct 13 2016, 1:28 PM
Abe added a subscriber: llvm-commits.
rengolin edited edge metadata.Oct 13 2016, 1:34 PM

Hi Abe,

I see where you're coming from, but changing the command line options that the user has explicitly passed is a bit devious.

If we're going to change how we run on -Ofast/-ffast-math, I'd rather we didn't even run those tests than run them without the flags.

Principle of least surprise. :)

cheers,
--renato

MatzeB edited edge metadata.Oct 13 2016, 1:43 PM

I think we should uphold as a general principle that things like this are configurable from the outside and the automatic detection schemes are only used to set default values. This means using
set(VARNAME ... CACHE ...) in cmake to get a user configurable parameter. So that when the autodetection fails you have a quick and easy way to override it.

Did you mean to only present the minisat makefile here? This seems to contain unrelated changes to cmake/modules/SingleMultiSource.cmake and crudely disables fast-math and fp-contraction in a lot of other benchmarks...

Abe updated this revision to Diff 74598.Oct 13 2016, 4:53 PM
Abe retitled this revision from test-suite: try to force many FP-dependent tests to compile with unaggressive FP optimization, add support for per-ISA reference outputs to test-suite: try to force many FP-dependent tests to compile with unaggressive FP optimization or not at all, add support for per-ISA reference outputs.
Abe edited edge metadata.

removed attempts to force no fast-math, made it detect fast-math and disable tests accordingly instead

Abe updated this object.Oct 13 2016, 4:56 PM

I would accept the FAST_MATH changes on their own, however I don't want to accept the fp-contract stuff while the discussion about that is still ongoing.

CMakeLists.txt
191

I would rename the final variable to something like "TEST_SUITE_USES_FAST_MATH" (so the name is also correct in the case of the variable being set by the user instead of automatically detected). From that point you should be able to handle it in a similar fashion as TEST_SUITE_BENCHMARKING_ONLY.

MultiSource/Applications/minisat/CMakeLists.txt
1–6

This can simply be if(NOT TEST_SUITE_USES_FAST_MATH) (or however you call the variable).

You could also consider applying at the points where llvm_add_subdirectories() is called (grep for how TEST_SUITE_BENCHMARKING_ONLY is used, this should simply some things like the TSVC benchmark where you could simply exclude the TSVC directory at the toplevel).

cmake/modules/SingleMultiSource.cmake
103–126

While this looks like a useful addition, it does not appear to be used here, is it?

Hi Abe,

Doesn't D25346 deprecate this change? At least some of it?

cheers,
--renato

sebpop edited edge metadata.Oct 16 2016, 8:07 AM

Hi Abe,

Doesn't D25346 deprecate this change? At least some of it?

Yes, all the polybench should be fixed now for -ffp-contract=on and there are still 2 tests to be fixed for -ffast-math.
Also in the description of this patch Abe mentioned:

[Abe is intentionally leaving the Polybench stuff for Sebastian since he has already started fixing it]

Abe, please rebase your patch on top of trunk, and remove all the changes to Polybench as they are not needed.

Abe updated this revision to Diff 74909.Oct 17 2016, 2:59 PM
Abe updated this object.
Abe edited edge metadata.

rebased to:

commit d471bba591ad51ecf4180cebda63e486d2fa877c
Author: Sunil Srivastava <sunil_srivastava@playstation.sony.com>
Date:   Mon Oct 17 04:32:15 2016 +0000

  Fixed line Endings.

  git-svn-id: https://llvm.org/svn/llvm-project/test-suite/trunk@284358 91177308-0d34-0410-b5e6-96231b3b80d8

... resolved conflicts, made some other small improvements.

Tested to pass on both AArch64 and AMD64 with both "-O3" and "-O3 -ffp-contract=fast".

sebpop added inline comments.Oct 17 2016, 3:04 PM
SingleSource/Benchmarks/Polybench/datamining/correlation/CMakeLists.txt
5

We do not need changes to Polybench.
Please remove all the changes to the CMake/Makefiles in all subdirectories of Polybench.

Abe retitled this revision from test-suite: try to force many FP-dependent tests to compile with unaggressive FP optimization or not at all, add support for per-ISA reference outputs to test-suite: disable tests that fail when FP optimization is in effect, add support for per-ISA reference outputs.Oct 18 2016, 1:09 PM

Sorry (again) for being late to this.

My point earlier was: haven't we found a better way to work around the contraction?

Abe abandoned this revision.Dec 14 2016, 12:00 PM