[PATCH] D24925: [testsuite] turn fp-contract off for ARM because output checking is not flexible enough

[Renato Golin via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D24925#552646, @spatel wrote:

> I don't know what "segregating the implementations per architecture" means, and this patch does not disable any testing (just makes it test differently). Let me try to explain some more:


This is exactly what it means: testing different things for different architectures. If it used to work, and it's not working, disabling / redirecting the tests is *not* a solution.

> 1. https://reviews.llvm.org/rL282259 (like https://reviews.llvm.org/D14200 / https://reviews.llvm.org/rL253269 before it) proposed to make -ffp-contract=on the default.

> 2. This allows FMA codegen for targets that have FMA instructions.

> 3. Please don't confuse this with -ffast-math; this has nothing to do with -ffast-math.

> 4. Obviously AArch64 has FMA instructions because r282259 caused AArch64 bots that run test-suite to fail and so it was reverted: https://reviews.llvm.org/rL282289.


So does ARM and many other architectures.

> 5. The output checking for some tests that use FP in test-suite appears to be looking for exact bit patterns and/or does not have an appropriate FP tolerance setting.

> 6. This output checking is broken when we use FMA instructions because FMA instructions can produce different results than FMUL + FADD.


As expected.

> 7. To keep the bots green, I am proposing (really just re-proposing the suggestions of Sept 23 from the post-commit thread for r282259) that they run with -ffp-contract=off in order to match the (faulty) expectations contained in test-suite.


And I'm strongly against any decision that disable features / testing patterns when trying to understand and fix the test / code is a viable solution. If everything was broken, I'd accept pushing hack patches in. This is not the case.

> 8. I'm actually surprised that other architectures would not also fail test-suite for the same reason. Maybe it's just that there are no other FMA-capable bots running test-suite?


That is most likely the case. Or they just happen to not break on that particular situation. Relying on unknown behaviour is dangerous.

> 9. It is not wrong for bots to run with a particular -ffp-contract setting; they are all perfectly valid modes of operation after all.


No, but this is a BOT option, not a TEST option. Unless there is *no* way to fix the test (which I haven't heard anything about it, yet), this could be one way of doing it.

But this is orthogonal to the decision itself.

> 10. Ideally, we would have more configs/bots to check each fp-contract mode. If we did that, then we'd also need to update the test-suite output checks to account for those different modes, but AFAIK nobody has volunteered to do that work.

> 11. Another solution would be to loosen the checks (increase the FP tolerance) in test-suite for tests that are sensitive to FMA codegen, so they will pass independently of the -ffp-contract setting.


Loosen the tests by increasing the tolerance would only be a viable alternative if we can prove that it is indeed irrelevant to the test at hand. Steve says it is, but he also says someone with time could fix the test.

Since nothing is broken and this is an optimisation, I think we do have the time to fix it.

> 12. This patch does not hinder anyone from doing the work for either #10 / #11.


It relaxes tests when we should be fixing tests. The first step of making tests irrelevant is disabling them for random reasons instead of fixing them. If that's a route we're willing to take, than we might just as well disable all buildbots.


https://reviews.llvm.org/D24925