This is an archive of the discontinued LLVM Phabricator instance.

test-suite: by default, assume that FP contraction is on unless the user specifies otherwise, disable tests accordingly; add support for per-ISA reference outputs
Needs ReviewPublic

Authored by Abe on Oct 18 2016, 1:48 PM.

Details

Summary

As you can see in the patch where I wrote a program to detect FP contraction, while it is possible to detect this optimization programmatically and act accordingly, because the compiled program must run on the test target, it is nontrivial to make all the relevant tests depend on the outcome of running such a program.

Sebastian and I therefor propose that we shall have a variable that is checked, such that the variable disables the FP-dependent test by default, but it can be re-enabled by setting the appropriate variable to an appropriate value [e.g. "no"].

Diff Detail

Event Timeline

Abe updated this revision to Diff 75067.Oct 18 2016, 1:48 PM
Abe retitled this revision from to test-suite: FP: by default, assume either contraction, fast-math, or both are on unless the user specifies otherwise, disable tests accordingly.
Abe updated this object.
Abe added reviewers: sebpop, rengolin, MatzeB.
Abe updated this object.Oct 18 2016, 1:51 PM
Abe updated this revision to Diff 75188.Oct 19 2016, 11:57 AM

added the bulk of it

Abe retitled this revision from test-suite: FP: by default, assume either contraction, fast-math, or both are on unless the user specifies otherwise, disable tests accordingly to test-suite: FP: by default, assume either contraction, fast-math, or both are on unless the user specifies otherwise, disable tests accordingly; add support for per-ISA reference outputs.Oct 19 2016, 11:57 AM
Abe updated this revision to Diff 75247.Oct 19 2016, 3:47 PM

minor edit re "minisat".

rengolin edited edge metadata.Oct 20 2016, 11:59 AM

So, two things I can see with the patch.

  1. A || B || Both is redundant. :) A || B should suffice.
  1. The plan was to only disable some tests with -Ofast, not with FP-contract=on.
MatzeB edited edge metadata.Oct 20 2016, 1:36 PM

Does this supersede https://reviews.llvm.org/D25575? Most of my comments there still apply here.

  • Make it a user configurable cmake variable (= use the SET CACHE variant). Try running ccmake or cmake -LAH to see what that is good for.
  • The cmake variable name should start with TEST_SUITE as the other variables we use as that is those variables will automatically be passed through from the toplevel buildsystem when you build the test-suite as part of llvm.
  • Read the cmake docu about "if" and what constitutes a false/true constant. You should use those conventions and simplify the string(TOLOWER... + if(MYVARIABLE STREQUAL "no" OR NOT DEFINED MYVARIABLE) to a simple if(NOT MYVARIABLE)!
  • Didn't we agree that fast-math and contraction are separate problems? I am surprised to see them tracked in a single variable.
  • I am confused by the alternate output for SingleSource/Benchmarks/Polybench/linear-algebra/solvers/dynprog/AArch64. Is this the result with fp-contraction enabled? Won't this output be invalid if the test is compiled with contraction disabled?
Abe updated this revision to Diff 75359.Oct 20 2016, 2:55 PM
Abe edited edge metadata.

Applied several requests/suggestions from MB & RG.

So, let's separate the FP_CONTRACT from the FAST_MATH problems, and solve them separately, in separate patches.

We may want to avoid some tests with -Ofast or -ffast-math, but we don't want to avoid them with -ffp-contract, at least not as a first attempt.

So, remove any trace of fp-contract from this patch and let's look at it from a fast-math perspective. Then we'll look at the fp problem in a separate patch, which should not be similar to this one as a first attempt, but something similar to what Sebastian did on his Polybench patch.

cheers,
--renato

Abe retitled this revision from test-suite: FP: by default, assume either contraction, fast-math, or both are on unless the user specifies otherwise, disable tests accordingly; add support for per-ISA reference outputs to test-suite: by default, assume that FP contraction is on unless the user specifies otherwise, disable tests accordingly; add support for per-ISA reference outputs.Oct 20 2016, 3:05 PM
Abe updated this revision to Diff 75360.Oct 20 2016, 3:09 PM

Renamed variables.

rengolin added inline comments.Oct 21 2016, 1:08 AM
CMakeLists.txt
191

Didn't you mean FAST_MATH?

Also, the explanatory text above still mentions both.

Abe updated this revision to Diff 75438.Oct 21 2016, 8:40 AM
Abe marked an inline comment as done.

minor help-text edit

Abe updated this object.Oct 21 2016, 8:44 AM

Right, this is using the other tool I just commented on. I think we need to be careful here.

We want to be able to ignore FP contraction issues when running the tests, but we also want to make sure we're not "contracting too much".

As I said earlier, and as we have found ways around elsewhere: it's better to understand each and every case and make a special decision for each one of them, instead of just blindly avoiding to run them when some contraction happens.

I mean, we *want* FP contraction to be meaningful and correct. If we just stop running these tests or if we change their errors to 1e10, we'll be effectively throwing *all* babies with the bath water.

So, I'm proposing we discard *all* patches disabling tests when contract happens and fix every one of them like you guys have fixed before.

cheers,
--renato

MatzeB resigned from this revision.Aug 15 2017, 11:08 AM