[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

Renato Golin via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 6 08:22:19 PDT 2016


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.

> 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.

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.

> 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. With your proposal change, they won't.
It doesn't matter much why.

Users will use the compiler with the default configuration, and if we
start disabling the default configuration in all of our tests, then we
will never test it, and what the users will see will be a completely
different compiler than we're testing.

The fact that the tests comparison is not good says nothing about the
quality of the tests. Today, if we have any difference, we'll notice.
If we force it fp=off, then any difference that fp=on produces will be
*completely* ignored, including horrendous bugs.

You can't possibly be suggesting that this is a good idea.

cheers,
--renato


https://reviews.llvm.org/D25194





More information about the llvm-commits mailing list