Page MenuHomePhabricator

make “#pragma STDC FP_CONTRACT” on by default
ClosedPublic

Authored by Abe on Sep 12 2016, 4:35 PM.

Details

Summary

Clang has the default FP contraction setting of “-ffp-contract=on”, which doesn`t really mean “on” in the conventional sense of the word, but rather really means “according to the per-statement effective value of the relevant pragma”.

Before this patch, Clang has that pragma defaulting to “off”. Since the “-ffp-contract=on” mode is really an AND of two booleans and the second of them defaults to “off”, the whole thing effectively defaults to “off”. This patch changes the default value of the pragma to “on”, thus making the default pair of booleans (on, on) rather than (on, off). This makes FP optimization slightly more aggressive than before when not using either “-Ofast”, “-ffast-math”, or “-ffp-contract=fast”. Even with this patch the compiler still respects “-ffp-contract=off”.

As per a suggestion by Steve Canon, the added code does _not_ require “-O3” or higher. This is so as to try our best to preserve identical floating-point results for unchanged source code compiling for an unchanged target when only changing from any optimization level in the set (“-O0”, “-O1”, “-O2”, “-O3”) to any other optimization level in that set. “-Os” and “-Oz” seem to be behaving identically, i.e. should probably be considered a part of the aforementioned set, but I have not reviewed this rigorously. “-Ofast” is explicitly _not_ a member of that set.

Diff Detail

Repository
rL LLVM

Event Timeline

Abe updated this revision to Diff 71073.Sep 12 2016, 4:35 PM
Abe retitled this revision from to make “#pragma STDC FP_CONTRACT” on by default.
Abe updated this object.
Abe added reviewers: scanon, lattner, llvm-commits.
scanon edited edge metadata.Sep 12 2016, 4:40 PM

Abe, I had a patch to do the same thing last year (that we ended up backing out because it exposed a bug which has since been fixed). Your approach looks a bit different from the one that I took originally, and you seem to be missing some of the arm64 test changes that I had to put in:

https://reviews.llvm.org/D14200

Can you comment on the differences?

Abe updated this revision to Diff 71190.Sep 13 2016, 10:03 AM
Abe edited edge metadata.

Added improved testing [both as improvements to existing tests and 1 entirely-new test] {copied from / in imitation of} https://reviews.llvm.org/D14200.

Abe updated this revision to Diff 71195.Sep 13 2016, 10:21 AM

Fixed "fp_contract_7" of "clang/test/CodeGen/fp-contract-pragma.cpp" so it actually _does_ have multiple _not-provably-dead_ uses of the result of the multiplication [this nuance might be important in the future if a DCE pass will occur before the IR is tested].

Abe updated this revision to Diff 71209.Sep 13 2016, 11:05 AM

Patch tweaking: renamed path prefixes from "clang/" to "cfe/trunk/", added _lots_ more context. Now the context shows up in Phabricator rather than "Context not available."

Abe added a subscriber: cfe-commits.
Abe added a reviewer: yaxunl.Sep 16 2016, 12:25 PM
Abe updated this revision to Diff 71953.Sep 20 2016, 10:27 AM

Removed some comments that I felt were good for clarity but at least 2 people disagreed.

yaxunl edited edge metadata.Sep 20 2016, 10:36 AM

Is it possible to merge cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O[0-3]___aarch64-backend.c as one and remove cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.h? They look the same except the RUN commands. Why not using one file with multiple RUn commands?

Abe added a comment.Sep 20 2016, 10:43 AM

Is it possible to merge cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O[0-3]___aarch64-backend.c as one and remove cfe/trunk/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.h? They look the same except the RUN commands. Why not using one file with multiple RUn commands?

That seems like a good idea. I didn`t write it that way yet b/c I was unsure of the semantics of multiple "RUN" command-comments in a single file. If a single file has e.g.:

// RUN: %clang_cc1 -triple aarch64 -O1 -S -o - %s | FileCheck %s
// RUN: %clang_cc1 -triple aarch64 -O2 -S -o - %s | FileCheck %s
// RUN: %clang_cc1 -triple aarch64 -O3 -S -o - %s | FileCheck %s

... then that will cause 3 compilations and 3 tests, right?

... then that will cause 3 compilations and 3 tests, right?

Right.

Abe updated this revision to Diff 71959.Sep 20 2016, 11:28 AM
Abe edited edge metadata.

Collapsed 4 test-case files that didn`t really need to be separate into 1 file, as suggested by Yaxun Liu.

yaxunl added inline comments.Sep 21 2016, 7:35 AM
clang/test/CodeGen/fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.c
8 ↗(On Diff #71959)

This check seems unnecessary and can be removed. The same as below. Then this test can be merged with fp-contract-pragma___on-by-default___-O0___aarch64-backend.c

Abe updated this revision to Diff 72103.Sep 21 2016, 2:02 PM

Combined "fp-contract-pragma___on-by-default___-O0___aarch64-backend.c" and "fp-contract-pragma___on-by-default___-O1...3___aarch64-backend.c" into a single file ["fp-contract-pragma___on-by-default.c"].

yaxunl added inline comments.Sep 21 2016, 2:24 PM
clang/lib/Frontend/CompilerInvocation.cpp
2441 ↗(On Diff #72103)

The format of this line looks strange. Did you check with clang-format-diff.py if it is OK?

Abe updated this revision to Diff 72186.Sep 22 2016, 9:42 AM

Minor edits for style-guidelines conformance.

LGTM. Thanks.

scanon accepted this revision.Sep 22 2016, 10:44 AM
scanon edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 22 2016, 10:44 AM
This revision was automatically updated to reflect the committed changes.

Folks, this commit has broken both AArch64 test-suite buildbots:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/3162

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/10449

I have reverted in r282289, let me know if you need help testing on AArch64.