This patch is designed to enable the compiler [Clang] patch at https://reviews.llvm.org/D24481 to go back into master/trunk and stay there without breaking the test-suite build-bots. Since at least some of the existing test-suite tests are based on the exactly-identical-output-or-fail philosophy, it will take some time and some work to enable test-suite to really perform FP comparisons properly, i.e. with a positive tolerance to allow for small FP variations due to FP optimization. The "fpcmp" program that is already in place is able to help with this, but it is not yet used consistently enough to give us the flexibility we need in order to be able to do more-and-better FP optimization and still have tests pass that should pass.
Diff Detail
Diff Detail
Event Timeline
Comment Actions
It looks like all the benchmarks that need adjustment are written in C.
Could you please remove the CXXFLAGS lines and only leave the CFLAGS change?
Otherwise the change looks good to me.
Comment Actions
What you are asking for is a new feature: testing anything other than -ffp-contract=off is currently unsupported by the test-suite. We agreed to help implement a feature that tests -ffp-contract=on and fast. Adding this new feature should not stand in our way to commit https://reviews.llvm.org/D24481
I would be interested to hear whether you have technical reasons to oppose this change.
Comment Actions
I have given technical reasons on the other review, namely:
- This is not a critical change that warrants modifying the tests for it.
- The tests are bad and need changing. If we don't change it now, they'll never get fixed.
- Changing the test to disable the feature that you want to enable will test something that people won't use in real life.
- Abe has proposed the best solution of them all: run both modes. We should do that first, then make sure neither break.
Reason number 3 is the critical one, that makes the other three very relevant.
cheers,
--renato
Comment Actions
The patch does not modify what the tests are currently testing for.
- The tests are bad and need changing. If we don't change it now, they'll never get fixed.
The tests are not bad: they are testing -ffp-contract=off, and we will continue testing that in the future.
There are no tests yet for -ffp-contract=on, and for that we will implement what Abe and Hal proposed.
- Changing the test to disable the feature that you want to enable will test something that people won't use in real life.
The patch does not disable anything: it only makes explicit the flag that the test-suite is currently testing for.
- Abe has proposed the best solution of them all: run both modes. We should do that first, then make sure neither break.
We will help implement this new test-suite feature when testing with cmake+lit.
The Makefiles will have to be modified as in the current patch to disable fp-contract.
IMO there is no technical reason to add the new tests before we switch the default -ffp-contract=on.
Comment Actions
You want to change the compiler behaviour to default to fp=on (or fast), and the tests will remain on fp=off.
This means you'll be testing for the non-default behaviour, while everyone else will be using the default behaviour, therefore, we won't be testing what we ship.
The minimum requirement is to test what we ship, and if that's with fp=on, we need to make the test pass on its own right.
The current proposition is to make it identical with fp=off and within a threshold for fp=on. I think that's more than fair.
IMO there is no technical reason to add the new tests before we switch the default -ffp-contract=on.
Adding new features without tests, or weakening tests to make way for new features is explicitly against the developer's policy:
http://llvm.org/docs/DeveloperPolicy.html#test-cases
http://llvm.org/docs/DeveloperPolicy.html#quality
I'm not sure why you want to push the feature before the tests are complete.
cheers,
--renato
Comment Actions
I do not think that test-suite tests for what LLVM ships:
one example is that 50 tests fail on x86_64-linux by adding -Ofast to the CFLAGS.
What all these 50 test are doing is testing -ffp-contract=off.
IMO the test-suite should not fail just because somebody adds extra CFLAGS to it:
this is a good reason to add for all those 50 tests -ffp-contract=off -fno-fast-math.
Instead of continuing arguing with you over this, I went ahead and implemented what we discussed:
The current proposition is to make it identical with fp=off and within a threshold for fp=on. I think that's more than fair.
Comment Actions
Adding the comment here, as llvm-commits wasn't copied.
On 6 October 2016 at 15:59, Sebastian Pop <sebpop@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