Page MenuHomePhabricator

[clang] Rename -frounding-math to -fexperimental-rounding-math and add -frounding-math back as a gcc-compat arg.
AbandonedPublic

Authored by rupprecht on Dec 17 2019, 3:54 PM.

Details

Summary

D62731 implements an incomplete/experimental version of rounding math, but since the flag doesn't have "experimental" in the name, the feedback that it is experimental is in the form of -Wexperimental-float-control.

Since this flag is already being accepted as a gcc compatability arg, put the current implementation being -f[no-]experimental-rounding-math instead, and allow -frounding-math as an ignored arg. Because "experimental" is now in the flag name, remove the warning -- it's now clear that the feature is incomplete/experimental when invoking it.

Diff Detail

Unit TestsFailed

TimeTest
380 mslit.lit::shtest-format.py
Script: -- : 'RUN: at line 3'; rm -f /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/utils/lit/tests/Output/shtest-format.py.tmp.xml
380 mslit.lit::shtest-format.py
Script: -- : 'RUN: at line 3'; rm -f /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/utils/lit/tests/Output/shtest-format.py.tmp.xml

Event Timeline

rupprecht created this revision.Dec 17 2019, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 3:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Unit tests: fail. 60990 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: fail. 60990 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Before:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

CC1 will do rounding math things.

After

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

CC1 will not do rounding math things. -fexperimental-rounding-math if the user really wants to use the feature.

Is my understanding correct? If yes, this patch seems pretty reasonable to me, because -frounding-math is currently incomplete/unsafe.

You may consider not changing CC1 options as they are not user facing.

rupprecht abandoned this revision.Dec 18 2019, 5:36 PM

Before:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

CC1 will do rounding math things.

After

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

CC1 will not do rounding math things. -fexperimental-rounding-math if the user really wants to use the feature.

Is my understanding correct? If yes, this patch seems pretty reasonable to me, because -frounding-math is currently incomplete/unsafe.

You may consider not changing CC1 options as they are not user facing.

I landed D71671 instead, as apparently the -frounding-math option is safe, although perhaps incomplete

Before:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

CC1 will do rounding math things.

After

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

CC1 will not do rounding math things. -fexperimental-rounding-math if the user really wants to use the feature.

Is my understanding correct? If yes, this patch seems pretty reasonable to me, because -frounding-math is currently incomplete/unsafe.

You may consider not changing CC1 options as they are not user facing.

My understanding is this:

Before D62731:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

After D62731:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

After D71671:

% clang -frounding-math -fsyntax-only -x c /dev/null
<no warning>

I suppose this patch would be updated to put us back where we were before D62731 in terms of the warning and require an additional option for anyone who wants to use the rounding math implementation as it is currently implemented.

The support for -frounding-math is incomplete, but I'm not sure it's unsafe. It's no more unsafe than not using the flag at all. The same reasoning applies to -ftrapping-math which was accepted without warning well before D62731 and has never been completely implemented.