Skip to content

[Clang][doc] -ffp-contract options and standard compliance #127621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

sjoerdmeijer
Copy link
Collaborator

We had an internal discussion about -ffp-contract, how it compared to GCC which defaults to fast, and standard compliance. Looking at our docs, I think most information is there, but also thought it could be a little bit more explicit about when it is and isn't standard compliant. Let me know if you think this is an improvement or not; happy to abandon this if you think it is not helping much.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Feb 18, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 18, 2025

@llvm/pr-subscribers-clang

Author: Sjoerd Meijer (sjoerdmeijer)

Changes

We had an internal discussion about -ffp-contract, how it compared to GCC which defaults to fast, and standard compliance. Looking at our docs, I think most information is there, but also thought it could be a little bit more explicit about when it is and isn't standard compliant. Let me know if you think this is an improvement or not; happy to abandon this if you think it is not helping much.


Full diff: https://github.com/llvm/llvm-project/pull/127621.diff

1 Files Affected:

  • (modified) clang/docs/UsersManual.rst (+14-8)
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index d977868b8a2c6..8809632479a00 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -1681,19 +1681,25 @@ for more details.
    permitted to produce more precise results than performing the same
    operations separately.
 
-   The C standard permits intermediate floating-point results within an
+   The C/C++ standard permits intermediate floating-point results within an
    expression to be computed with more precision than their type would
    normally allow. This permits operation fusing, and Clang takes advantage
-   of this by default. This behavior can be controlled with the ``FP_CONTRACT``
-   and ``clang fp contract`` pragmas. Please refer to the pragma documentation
-   for a description of how the pragmas interact with this option.
+   of this by default (``on``). Fusion across statements is non-C/C++ standard
+   compliant behavior and can be enabled (``fast``).
+
+   Fusion can be controlled with the ``FP_CONTRACT`` and ``clang fp contract``
+   pragmas. Please refer to the pragma documentation for a description of how
+   the pragmas interact with this option.
 
    Valid values are:
 
-   * ``fast`` (fuse across statements disregarding pragmas, default for CUDA)
-   * ``on`` (fuse in the same statement unless dictated by pragmas, default for languages other than CUDA/HIP)
-   * ``off`` (never fuse)
-   * ``fast-honor-pragmas`` (fuse across statements unless dictated by pragmas, default for HIP)
+   * ``fast``: enable non-C/C++ standard compliant behavior and fusion across
+     statements disregarding pragmas (default for CUDA)
+   * ``on``: enable C/C++ standard complaint fusion in the same statement unless
+     dictated by pragmas (default for languages other than CUDA/HIP)
+   * ``off``: disable fusion
+   * ``fast-honor-pragmas``: fuse across statements unless dictated by pragmas
+     (default for HIP)
 
 .. option:: -f[no-]honor-infinities
 

Copy link
Contributor

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement. I have a few minor comments.

and ``clang fp contract`` pragmas. Please refer to the pragma documentation
for a description of how the pragmas interact with this option.
of this by default (``on``). Fusion across statements is non-C/C++ standard
compliant behavior and can be enabled (``fast``).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last sentence is a bit unclear. I would suggest " Fusion across statements is not compliant with the C/C++ standard but can be enabled using -ffp-contract=fast."

BTW, the C standard allows contraction of "expressions" saying that "a floating expression may be contracted" but the language gets imprecise if we start talking about "contraction across expressions" or "contractions across statements." I think saying "contractions across statements" is not allowed is more accurate, because we always have sub-expressions within what we're contracting. On the other hand, there are cases where we will not contract even within a statement. For example, an explicit cast in the middle of a statement prevents contraction (e.g. x = c + (float)(a * b) when a, b, and c are double). @AaronBallman can you suggest more precise wording?

* ``off`` (never fuse)
* ``fast-honor-pragmas`` (fuse across statements unless dictated by pragmas, default for HIP)
* ``fast``: enable non-C/C++ standard compliant behavior and fusion across
statements disregarding pragmas (default for CUDA)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the order here. Fusion across expressions is the important point for most people. Perhaps "enable fusion across expressions, breaking compliance with the C/C++ standard (default for CUDA)"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I did notice you're using "expressions", but I've kept "statements".

Copy link
Collaborator Author

@sjoerdmeijer sjoerdmeijer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I have addressed the comments, I think.

* ``off`` (never fuse)
* ``fast-honor-pragmas`` (fuse across statements unless dictated by pragmas, default for HIP)
* ``fast``: enable non-C/C++ standard compliant behavior and fusion across
statements disregarding pragmas (default for CUDA)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I did notice you're using "expressions", but I've kept "statements".

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy when @andykaylor is.

@@ -1681,19 +1681,27 @@ for more details.
permitted to produce more precise results than performing the same
operations separately.

The C standard permits intermediate floating-point results within an
The C/C++ standard permits intermediate floating-point results within an
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The C/C++ standard permits intermediate floating-point results within an
The C/C++ standards permit intermediate floating-point results within an

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer C and C++ standards. Same elsewhere in the changes. :-)

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, with a nit about C/C++

@@ -1681,19 +1681,27 @@ for more details.
permitted to produce more precise results than performing the same
operations separately.

The C standard permits intermediate floating-point results within an
The C/C++ standard permits intermediate floating-point results within an
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer C and C++ standards. Same elsewhere in the changes. :-)

Copy link
Contributor

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the latest changes, so after the "standards" wording changes Erich and Aaron requested, this should be good to go.

@sjoerdmeijer
Copy link
Collaborator Author

Thanks for your reviews! I will make those changes before merging this.

We had an internal discussion about -ffp-contract, how it compared to
GCC which defaults to fast, and standard compliance. Looking at our
docs, I think most information is there, but also thought it could be a
little bit more explicit about when it is and isn't standard compliant.
Let me know if you think this is an improvement or not; happy to abandon
this if you think it is not helping much.
@sjoerdmeijer sjoerdmeijer merged commit dc326d0 into llvm:main Feb 20, 2025
9 checks passed
@sjoerdmeijer sjoerdmeijer deleted the docs-fp-contract branch February 20, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants