-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Always honor fp-contract pragmas on PlayStation #162549
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-backend-x86 Author: None (wjristow) ChangesThe pragma: can be used to disable FMA, but when cross-statement FMA is enabled in To have Clang honor that pragma in 'fast' fp contract mode in C/C++, an additional switch: must be applied. On PlayStation, we want to always honor that pragma, without requiring the additional switch to be specified. This commit does that. Full diff: https://github.com/llvm/llvm-project/pull/162549.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 64f1917739e12..52bf702cdadb0 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -396,7 +396,11 @@ static bool initTargetOptions(const CompilerInstance &CI,
Options.AllowFPOpFusion = llvm::FPOpFusion::Standard;
break;
case LangOptions::FPM_Fast:
- Options.AllowFPOpFusion = llvm::FPOpFusion::Fast;
+ // We always honor fp-contract pragmas for PlayStation.
+ if (CI.getASTContext().getTargetInfo().getTriple().isPS())
+ Options.AllowFPOpFusion = llvm::FPOpFusion::Standard;
+ else
+ Options.AllowFPOpFusion = llvm::FPOpFusion::Fast;
break;
}
diff --git a/clang/test/CodeGen/X86/fma-fast-pragma.cpp b/clang/test/CodeGen/X86/fma-fast-pragma.cpp
new file mode 100644
index 0000000000000..9d38e245b5f64
--- /dev/null
+++ b/clang/test/CodeGen/X86/fma-fast-pragma.cpp
@@ -0,0 +1,53 @@
+// REQUIRES: x86-registered-target
+
+// With the pragma in place, generic targets leave FMA enabled unless the
+// switch '-ffp-contract=fast-honor-pragmas' is used to disable it; whereas
+// for PlayStation, the pragma is always honored, so FMA is disabled even in
+// plain 'fast' mode:
+// RUN: %clang_cc1 -S -triple x86_64-unknown-unknown -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-YES-FMA %s
+// RUN: %clang_cc1 -S -triple x86_64-unknown-unknown -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast-honor-pragmas -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-NO-FMA %s
+// RUN: %clang_cc1 -S -triple x86_64-unknown-unknown -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast -ffp-contract=fast-honor-pragmas -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-NO-FMA %s
+// RUN: %clang_cc1 -S -triple x86_64-sie-ps5 -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-NO-FMA %s
+// RUN: %clang_cc1 -S -triple x86_64-sie-ps5 -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast-honor-pragmas -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-NO-FMA %s
+//
+// With the pragma suppressed, FMA happens in 'fast' or 'fast-honor-pragmas'
+// modes (for generic targets and for PlayStation):
+// RUN: %clang_cc1 -S -DSUPPRESS_PRAGMA \
+// RUN: -triple x86_64-unknown-unknown -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-YES-FMA %s
+// RUN: %clang_cc1 -S -DSUPPRESS_PRAGMA \
+// RUN: -triple x86_64-unknown-unknown -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast-honor-pragmas -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-YES-FMA %s
+// RUN: %clang_cc1 -S -DSUPPRESS_PRAGMA \
+// RUN: -triple x86_64-sie-ps5 -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-YES-FMA %s
+// RUN: %clang_cc1 -S -DSUPPRESS_PRAGMA \
+// RUN: -triple x86_64-sie-ps5 -target-feature +fma \
+// RUN: -O2 -ffp-contract=fast-honor-pragmas -o - %s | \
+// RUN: FileCheck --check-prefix=CHECK-YES-FMA %s
+//
+float compute(float a, float b, float c) {
+#if !defined(SUPPRESS_PRAGMA)
+#pragma clang fp contract (off)
+#endif
+ float product = a * b;
+ return product + c;
+}
+
+// CHECK-NO-FMA: vmulss
+// CHECK-NO-FMA-NEXT: vaddss
+
+// CHECK-YES-FMA: vfmadd213ss
|
|
The test-case of this patch generates X86 assembly code, rather than the usual approach of generating generic IR. The reason I did that is because the impact of the change isn't visible in the IR. That's due to a bug, which I've just filed: #162550 If this patch is accepted, we will want to update the documentation of For reference, here are some points of discussion about the |
|
I continue to believe that ignoring pragmas when fast-math is enabled is a bug, and I'd like to see a more general change such as I proposed in |
The pragma:
#pragma clang fp contract (off)
can be used to disable FMA, but when cross-statement FMA is enabled in
`fast` fp contract mode (via `-ffast-math`, for example), the effect of
that pragma is suppressed.
To have Clang honor that pragma in 'fast' fp contract mode in C/C++, an
additional switch:
-ffp-contract=fast-honor-pragmas
must be applied. On PlayStation, we want to always honor that pragma,
without requiring the additional switch to be specified. This commit
does that.
9e1b410 to
123ddd9
Compare
FWIW, that makes good sense to me. I admit I only looked at that PR of yours carefully today, as I was preparing this one. |
|
Maybe time to revisit #105746; the situation in the backends looks a lot better. Looks like Arm and Mips still have some problematic isel patterns; everything else looks okay at first glance? We could blacklist specific backends which still have issues. |
That sounds good to me. Like Andy, I also view the current behavior as a bug. And I view this PR of mine as "fix that bug for PlayStation", whereas I view #105746 as "fix that bug, for all". So I'd be happy to have that done. And as an aside, if we do Andy's full fix, then the bug I reported as related to this (#162550) becomes moot. That bug is essentially: In LTO mode the pragma is always honored. Or more directly, the switch |
|
Given @andykaylor's comment:
and @efriedma-quic's comment:
I think revisiting that is the right thing to do. That said, if complications on some specific backends make that difficult, we could move forward with this PR, fixing this on PlayStation (in our downstream version, we're already doing that). Regarding blacklisting specific backends, if we were to do this PR, we could also do the dual of that blacklist approach: That is, others that want the pragmas to always be honored could opt-in to the approach proposed here in this PR. I've just added a note in Andy's PR #105746, suggesting these two possibilities. |
| break; | ||
| case LangOptions::FPM_Fast: | ||
| Options.AllowFPOpFusion = llvm::FPOpFusion::Fast; | ||
| // We always honor fp-contract pragmas for PlayStation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should simply do this silently (which may be surprising) or if we should at least issue a warning that we are doing this? Perhaps with a deprecation notice in the vague hope that we could turn it into an error in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it happens, for PlayStation, we have had a private patch in place to honor the fp-contract pragma since our llvm11-based release (this was before the concept of -ffp-contract=fast-honor-pragmas was created). So this isn't a change in behavior for PlayStation developers, and so no need for a warning, from that perspective.
That said, if #105746 is done in some form (and there is activity there now), this PR can be abandoned. And as I've said above, I'm very much in favor of that approach being taken.
The pragma:
#pragma clang fp contract (off)
can be used to disable FMA, but when cross-statement FMA is enabled in
fastfp contract mode (via-ffast-math, for example), the effect of that pragma is suppressed.To have Clang honor that pragma in 'fast' fp contract mode in C/C++, an additional switch:
-ffp-contract=fast-honor-pragmas
must be applied. On PlayStation, we want to always honor that pragma, without requiring the additional switch to be specified. This commit does that.