[PATCH] D25194: Change some of the Makefiles of the Makefile-based test harness to use "-ffp-contract=off" so the build-bots will be able to tolerate more-aggressive FP optimization by default

[Sebastian Pop via llvm-commits]

On Thu, Oct 6, 2016 at 11:22 AM, Renato Golin via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> rengolin added a comment.
>
> Adding the comment here, as llvm-commits wasn't copied.
>
>> On 6 October 2016 at 15:59, Sebastian Pop <sebpop at gmail.com> wrote:
>>  As you can see, this comes with its share amount of troubles...
>>  mostly because some benchmarks in the test-suite were not designed
>>  with testing for FP_TOLERANCE.
>>  There are ways to modify these benchmarks to add a mode to correctly
>>  test for FP, though that is a very long development
>
> Hi Sebastian,
>
> It is, indeed, a long development, but such are the perils of open
> source development.
>
>> I would like to go ahead and fix the current state of the test-suite,
>>  by explicitly adding "-fno-fast-math -ffp-contract=off" for all the 50
>>  benchmarks failing with -Ofast.
>
> I hope I made clear that I'm absolutely against that.
>
> I'm finding it hard to understand why is this not clear to you.
>
> You internal pressures in your company cannot leak out upstream. If
> you need this patch now, keep it downstream until they work as
> intended, then submit them again.

No thank you!  I won't play that game anytime soon.
It will cost more in the long run than fixing the broken tests in the
test-suite right now.

>
>> This is not true: we do have tests correctly checking FP: for example
>>  the SPEC 2k and 2k6.
>
> I'm sorry, but that has to be a joke.

That of course was only one example of a benchmark that has been
designed to test FP.  It is not the only one in the test-suite: as I mentioned
there are 79 benchmarks with Makefiles setting FP_*TOLERANCE and
51 with CMake.

>From my testing, some of these benchmarks are currently broken
when testing with -ffp-contract=on or -ffast-math.

> Not only SPEC is a closed source benchmark, but even sharing the
> numbers is against the license. So, even if we had a downstream
> buildbot tracking those, we wouldn't be able to publish any of that as
> we do for the other buildbots.

You'll hear from me if someone breaks SPEC at -Ofast ;-)

>> There are of course more than 50 benchmarks testing FP:
>>  Makefiles specifiying FP_TOLERANCE: 48, FP_ABSTOLERANCE: 31
>>  CMakeLists.txt specifying FP_TOLERANCE: 15, FP_ABSTOLERANCE: 36
>>  As we already have tests that pass with -Ofast and -ffp-contract=on,
>>  there is no reason to say that we need to add tests for that.
>>  Adding more tests should not stand in our way to commit the patch
>>  turning on by default -ffp-contract=on.
>
> Right, here I go again...
>
> The current tests pass with the current default configuration of the
> compiler, for better or worse.

Partly true: please define "current default configuration".
I'm not sure I understand what you mean by "default".
The current tests in the test-suite only pass when not specifying any extra
CFLAGS at configure time.
There are currently 50 broken tests with CFLAGS=-Ofast on x86_64-linux.

> With your proposal change, they won't.  It doesn't matter much why.

I do not understand your point here, or you do not understand what I'm
proposing.

My proposal is to fix those 50 tests not passing with -Ofast and the
other tests not passing with -ffp-contract=on.
I want to have clean test results with the test-suite configured with
CFLAGS="", "-Ofast", "-ffp-contract=on".
These are the default flags that I am configuring the test-suite with
and the defaults I care for when testing,
and currently the test-suite is broken under these flags.

Sebastian