[clang] Lower non-builtin sincos[f|l] calls to llvm.sincos.* when -fno-math-errno is set #121763
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-clang Author: Benjamin Maxwell (MacDue) ChangesThis will allow vectorizing these calls (after a few more patches). This should not change the codegen for targets that enable the use of AA during the codegen (in Follow up to #114086. Full diff: https://github.com/llvm/llvm-project/pull/121763.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index c419fb0cc055e0..9a859e7a22f5e3 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -3264,6 +3264,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
return RValue::get(emitUnaryMaybeConstrainedFPBuiltin(
*this, E, Intrinsic::sinh, Intrinsic::experimental_constrained_sinh));
+ case Builtin::BIsincos:
+ case Builtin::BIsincosf:
+ case Builtin::BIsincosl:
case Builtin::BI__builtin_sincos:
case Builtin::BI__builtin_sincosf:
case Builtin::BI__builtin_sincosf16:
diff --git a/clang/test/CodeGen/AArch64/sincos.c b/clang/test/CodeGen/AArch64/sincos.c
index b77d98ceab4869..fde277716ddddb 100644
--- a/clang/test/CodeGen/AArch64/sincos.c
+++ b/clang/test/CodeGen/AArch64/sincos.c
@@ -1,5 +1,19 @@
-// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - | FileCheck --check-prefix=NO-MATH-ERRNO %s
-// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - | FileCheck --check-prefix=MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - -DUSE_BUILTIN | FileCheck --check-prefix=NO-MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - -DUSE_BUILTIN | FileCheck --check-prefix=MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - -DUSE_C_DECL | FileCheck --check-prefix=NO-MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - -DUSE_C_DECL | FileCheck --check-prefix=MATH-ERRNO %s
+
+#if defined(USE_BUILTIN)
+ #define sincos __builtin_sincos
+ #define sincosf __builtin_sincosf
+ #define sincosl __builtin_sincosl
+#elif defined(USE_C_DECL)
+ void sincos(double, double*, double*);
+ void sincosf(float, float*, float*);
+ void sincosl(long double, long double*, long double*);
+#else
+ #error Expected USE_BUILTIN or USE_C_DECL to be defined.
+#endif
// NO-MATH-ERRNO-LABEL: @sincos_f32
// NO-MATH-ERRNO: [[SINCOS:%.*]] = tail call { float, float } @llvm.sincos.f32(float {{.*}})
@@ -12,7 +26,7 @@
// MATH-ERRNO: call void @sincosf(
//
void sincos_f32(float x, float* fp0, float* fp1) {
- __builtin_sincosf(x, fp0, fp1);
+ sincosf(x, fp0, fp1);
}
// NO-MATH-ERRNO-LABEL: @sincos_f64
@@ -26,7 +40,7 @@ void sincos_f32(float x, float* fp0, float* fp1) {
// MATH-ERRNO: call void @sincos(
//
void sincos_f64(double x, double* dp0, double* dp1) {
- __builtin_sincos(x, dp0, dp1);
+ sincos(x, dp0, dp1);
}
// NO-MATH-ERRNO-LABEL: @sincos_f128
@@ -40,5 +54,5 @@ void sincos_f64(double x, double* dp0, double* dp1) {
// MATH-ERRNO: call void @sincosl(
//
void sincos_f128(long double x, long double* ldp0, long double* ldp1) {
- __builtin_sincosl(x, ldp0, ldp1);
+ sincosl(x, ldp0, ldp1);
}
|
|
@llvm/pr-subscribers-clang-codegen Author: Benjamin Maxwell (MacDue) ChangesThis will allow vectorizing these calls (after a few more patches). This should not change the codegen for targets that enable the use of AA during the codegen (in Follow up to #114086. Full diff: https://github.com/llvm/llvm-project/pull/121763.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index c419fb0cc055e0..9a859e7a22f5e3 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -3264,6 +3264,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
return RValue::get(emitUnaryMaybeConstrainedFPBuiltin(
*this, E, Intrinsic::sinh, Intrinsic::experimental_constrained_sinh));
+ case Builtin::BIsincos:
+ case Builtin::BIsincosf:
+ case Builtin::BIsincosl:
case Builtin::BI__builtin_sincos:
case Builtin::BI__builtin_sincosf:
case Builtin::BI__builtin_sincosf16:
diff --git a/clang/test/CodeGen/AArch64/sincos.c b/clang/test/CodeGen/AArch64/sincos.c
index b77d98ceab4869..fde277716ddddb 100644
--- a/clang/test/CodeGen/AArch64/sincos.c
+++ b/clang/test/CodeGen/AArch64/sincos.c
@@ -1,5 +1,19 @@
-// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - | FileCheck --check-prefix=NO-MATH-ERRNO %s
-// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - | FileCheck --check-prefix=MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - -DUSE_BUILTIN | FileCheck --check-prefix=NO-MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - -DUSE_BUILTIN | FileCheck --check-prefix=MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -O1 %s -o - -DUSE_C_DECL | FileCheck --check-prefix=NO-MATH-ERRNO %s
+// RUN: %clang_cc1 -triple=aarch64-gnu-linux -emit-llvm -fmath-errno %s -o - -DUSE_C_DECL | FileCheck --check-prefix=MATH-ERRNO %s
+
+#if defined(USE_BUILTIN)
+ #define sincos __builtin_sincos
+ #define sincosf __builtin_sincosf
+ #define sincosl __builtin_sincosl
+#elif defined(USE_C_DECL)
+ void sincos(double, double*, double*);
+ void sincosf(float, float*, float*);
+ void sincosl(long double, long double*, long double*);
+#else
+ #error Expected USE_BUILTIN or USE_C_DECL to be defined.
+#endif
// NO-MATH-ERRNO-LABEL: @sincos_f32
// NO-MATH-ERRNO: [[SINCOS:%.*]] = tail call { float, float } @llvm.sincos.f32(float {{.*}})
@@ -12,7 +26,7 @@
// MATH-ERRNO: call void @sincosf(
//
void sincos_f32(float x, float* fp0, float* fp1) {
- __builtin_sincosf(x, fp0, fp1);
+ sincosf(x, fp0, fp1);
}
// NO-MATH-ERRNO-LABEL: @sincos_f64
@@ -26,7 +40,7 @@ void sincos_f32(float x, float* fp0, float* fp1) {
// MATH-ERRNO: call void @sincos(
//
void sincos_f64(double x, double* dp0, double* dp1) {
- __builtin_sincos(x, dp0, dp1);
+ sincos(x, dp0, dp1);
}
// NO-MATH-ERRNO-LABEL: @sincos_f128
@@ -40,5 +54,5 @@ void sincos_f64(double x, double* dp0, double* dp1) {
// MATH-ERRNO: call void @sincosl(
//
void sincos_f128(long double x, long double* ldp0, long double* ldp1) {
- __builtin_sincosl(x, ldp0, ldp1);
+ sincosl(x, ldp0, ldp1);
}
|
arsenm
reviewed
Jan 6, 2025
There was a problem hiding this comment.
@RKSimon do you know if there is a reason CodeGen AA is not enabled for x86?
I'd otherwise be happy to approve this patch, lowering to the intrinsic seems like the right thing to do.
I don't remember any reason for alias analysis to not be enabled on x86 - @phoebewang @topperc do you know? |
I don't remember either. |
Me neither. @clin111 do you happen to know it? |
MacDue
added a commit
to MacDue/llvm-project
that referenced
this pull request
Jan 21, 2025
This can still be disabled by setting the flag `-x86-use-aa=false`. All tests have been updated to account for this change except: test/CodeGen/X86/regalloc-advanced-split-cost.ll Where the spill needed for part of the test disappears with codegen AA enabled (so it is left disabled for that test). Enabling AA during codegen makes X86 consistent with other targets such as AArch64 and RISC-V. This will avoid regressing x86 targets when using the new `llvm.sincos` intrinsic see: llvm#121763
I've created a patch enabling it here: #123787 (maybe you'll spot why it's disabled there 😅). I've updated the generated tests, and gone over the handwritten ones more carefully, and updated them myself. The handwritten tests that changed were: Someone more experienced with x86(_64) should double check the changes make sense. I am fairly confident the underlying transforms should be correct though as this feature is enabled for other targets (at least AArch64 and RISC-V, off the top of my head). |
nikic
pushed a commit
to nikic/llvm-project
that referenced
this pull request
Jan 21, 2025
This can still be disabled by setting the flag `-x86-use-aa=false`. All tests have been updated to account for this change except: test/CodeGen/X86/regalloc-advanced-split-cost.ll Where the spill needed for part of the test disappears with codegen AA enabled (so it is left disabled for that test). Enabling AA during codegen makes X86 consistent with other targets such as AArch64 and RISC-V. This will avoid regressing x86 targets when using the new `llvm.sincos` intrinsic see: llvm#121763
|
Looks like it was never enabled --- or tested on X86 --- since UseAA was introduced in https://reviews.llvm.org/D67266. Eventually the dead option was removed with c266776. Would advise some caution with regards to register pressure and compile time. |
MacDue
added a commit
to MacDue/llvm-project
that referenced
this pull request
Jan 23, 2025
This can still be disabled by setting the flag `-x86-use-aa=false`. All tests have been updated to account for this change except: test/CodeGen/X86/regalloc-advanced-split-cost.ll Where the spill needed for part of the test disappears with codegen AA enabled (so it is left disabled for that test). Enabling AA during codegen makes X86 consistent with other targets such as AArch64 and RISC-V. This will avoid regressing x86 targets when using the new `llvm.sincos` intrinsic see: llvm#121763
|
Given it looks like #123787 won't be accepted (due to compile time regressions), I think we should restrict this lowering to AArch64 (and other supported targets) until there's a solution for targets without AA in codegen too. Does that seem reasonable? |
…o-math-errno is set This will allow vectorizing these calls (after a few more patches). This should not change the codegen for targets that enable the use of AA during the codegen (in `TargetSubtargetInfo::useAA()`). This includes targets such as AArch64. This notably does not include x86 but can be worked around by passing `-mllvm -combiner-global-alias-analysis=true` to clang.
MacDue
force-pushed
the
sincos_builtin_2
branch
from
46aa023 to
aafed97
Compare
|
I've pushed a change to restrict this to AArch64 (other targets could enable it too) given no reply above, or progress on #123787. |
arsenm
requested changes
Feb 18, 2025
This reverts commit aafed97.
Fine, I don't mind the regression as it does not affect targets I work on, I just wanted to make the changes needed for AArch64 without knowingly slightly regressing other targets. |
arsenm
approved these changes
Feb 18, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/7267 Here is the relevant piece of the build log for the reference |
Prakhar-Dixit
pushed a commit
to Prakhar-Dixit/llvm-project
that referenced
this pull request
Feb 19, 2025
…o-math-errno is set (llvm#121763) This will allow vectorizing these calls (after a few more patches). This should not change the codegen for targets that enable the use of AA during the codegen (in `TargetSubtargetInfo::useAA()`). This includes targets such as AArch64. This notably does not include x86 but can be worked around by passing `-mllvm -combiner-global-alias-analysis=true` to clang. Follow up to llvm#114086.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will allow vectorizing these calls (after a few more patches). This should not change the codegen for targets that enable the use of AA during the codegen (in
TargetSubtargetInfo::useAA()). This includes targets such as AArch64. This notably does not include x86 but can be worked around by passing-mllvm -combiner-global-alias-analysis=trueto clang.Follow up to #114086.