[Clang] [Sema] Fix bug in _Complex float+int arithmetic
#83063
Merged
Conversation
This file contains hidden or 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-clang Author: None (Sirraide) ChangesC23 6.3.1.8 ‘Usual arithmetic conversions’ p1 states (emphasis mine): ‘type domain’ here refers to Consider the following code: _Complex float f;
int x;
f / x;After talking this over with @AaronBallman, we came to the conclusion that This pr removes the extra The only exception here is Lastly, we were also missing tests for Full diff: https://github.com/llvm/llvm-project/pull/83063.diff 4 Files Affected:
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 4049ab3bf6cafb..c9647ee4a2938e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -1114,8 +1114,6 @@ static bool handleIntegerToComplexFloatConversion(Sema &S, ExprResult &IntExpr,
if (IntTy->isIntegerType()) {
QualType fpTy = ComplexTy->castAs<ComplexType>()->getElementType();
IntExpr = S.ImpCastExprToType(IntExpr.get(), fpTy, CK_IntegralToFloating);
- IntExpr = S.ImpCastExprToType(IntExpr.get(), ComplexTy,
- CK_FloatingRealToComplex);
} else {
assert(IntTy->isComplexIntegerType());
IntExpr = S.ImpCastExprToType(IntExpr.get(), ComplexTy,
diff --git a/clang/test/CodeGen/complex-math-mixed.c b/clang/test/CodeGen/complex-math-mixed.c
new file mode 100644
index 00000000000000..44aa83fa1c0d10
--- /dev/null
+++ b/clang/test/CodeGen/complex-math-mixed.c
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 %s -O0 -emit-llvm -triple x86_64-unknown-unknown -o - | FileCheck %s --check-prefix=X86
+
+// Check that for 'F _Complex + int' (F = real floating-point type), we emit an
+// implicit cast from 'int' to 'F', but NOT to 'F _Complex' (i.e. that we do
+// 'F _Complex + F', NOT 'F _Complex + F _Complex'), and likewise for -/*.
+
+float _Complex add_float_ci(float _Complex a, int b) {
+ // X86-LABEL: @add_float_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fadd float {{.*}}, [[I]]
+ // X86-NOT: fadd
+ return a + b;
+}
+
+float _Complex add_float_ic(int a, float _Complex b) {
+ // X86-LABEL: @add_float_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fadd float [[I]]
+ // X86-NOT: fadd
+ return a + b;
+}
+
+float _Complex sub_float_ci(float _Complex a, int b) {
+ // X86-LABEL: @sub_float_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fsub float {{.*}}, [[I]]
+ // X86-NOT: fsub
+ return a - b;
+}
+
+float _Complex sub_float_ic(int a, float _Complex b) {
+ // X86-LABEL: @sub_float_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fsub float [[I]]
+ // X86: fneg
+ // X86-NOT: fsub
+ return a - b;
+}
+
+float _Complex mul_float_ci(float _Complex a, int b) {
+ // X86-LABEL: @mul_float_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fmul float {{.*}}, [[I]]
+ // X86: fmul float {{.*}}, [[I]]
+ // X86-NOT: fmul
+ return a * b;
+}
+
+float _Complex mul_float_ic(int a, float _Complex b) {
+ // X86-LABEL: @mul_float_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fmul float [[I]]
+ // X86: fmul float [[I]]
+ // X86-NOT: fmul
+ return a * b;
+}
+
+float _Complex div_float_ci(float _Complex a, int b) {
+ // X86-LABEL: @div_float_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: fdiv float {{.*}}, [[I]]
+ // X86: fdiv float {{.*}}, [[I]]
+ // X86-NOT: @__divsc3
+ return a / b;
+}
+
+// There is no good way of doing this w/o converting the 'int' to a complex
+// number, so we expect complex division here.
+float _Complex div_float_ic(int a, float _Complex b) {
+ // X86-LABEL: @div_float_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to float
+ // X86: call {{.*}} @__divsc3(float {{.*}} [[I]], float noundef 0.{{0+}}e+00, float {{.*}}, float {{.*}})
+ return a / b;
+}
+
+double _Complex add_double_ci(double _Complex a, int b) {
+ // X86-LABEL: @add_double_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fadd double {{.*}}, [[I]]
+ // X86-NOT: fadd
+ return a + b;
+}
+
+double _Complex add_double_ic(int a, double _Complex b) {
+ // X86-LABEL: @add_double_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fadd double [[I]]
+ // X86-NOT: fadd
+ return a + b;
+}
+
+double _Complex sub_double_ci(double _Complex a, int b) {
+ // X86-LABEL: @sub_double_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fsub double {{.*}}, [[I]]
+ // X86-NOT: fsub
+ return a - b;
+}
+
+double _Complex sub_double_ic(int a, double _Complex b) {
+ // X86-LABEL: @sub_double_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fsub double [[I]]
+ // X86: fneg
+ // X86-NOT: fsub
+ return a - b;
+}
+
+double _Complex mul_double_ci(double _Complex a, int b) {
+ // X86-LABEL: @mul_double_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fmul double {{.*}}, [[I]]
+ // X86: fmul double {{.*}}, [[I]]
+ // X86-NOT: fmul
+ return a * b;
+}
+
+double _Complex mul_double_ic(int a, double _Complex b) {
+ // X86-LABEL: @mul_double_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fmul double [[I]]
+ // X86: fmul double [[I]]
+ // X86-NOT: fmul
+ return a * b;
+}
+
+double _Complex div_double_ci(double _Complex a, int b) {
+ // X86-LABEL: @div_double_ci
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: fdiv double {{.*}}, [[I]]
+ // X86: fdiv double {{.*}}, [[I]]
+ // X86-NOT: @__divdc3
+ return a / b;
+}
+
+// There is no good way of doing this w/o converting the 'int' to a complex
+// number, so we expect complex division here.
+double _Complex div_double_ic(int a, double _Complex b) {
+ // X86-LABEL: @div_double_ic
+ // X86: [[I:%.*]] = sitofp i32 {{%.*}} to double
+ // X86: call {{.*}} @__divdc3(double {{.*}} [[I]], double noundef 0.{{0+}}e+00, double {{.*}}, double {{.*}})
+ return a / b;
+}
diff --git a/clang/test/CodeGen/volatile.cpp b/clang/test/CodeGen/volatile.cpp
index 38724659ad8a35..70f523b93852ed 100644
--- a/clang/test/CodeGen/volatile.cpp
+++ b/clang/test/CodeGen/volatile.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -O2 -triple=x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK
+// RUN: %clang_cc1 -O2 -triple=x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s
struct agg
{
int a ;
@@ -10,34 +10,32 @@ _Complex float cf;
int volatile vol =10;
void f0() {
const_cast<volatile _Complex float &>(cf) = const_cast<volatile _Complex float&>(cf) + 1;
-// CHECK: %cf.real = load volatile float, ptr @cf
-// CHECK: %cf.imag = load volatile float, ptr getelementptr
-// CHECK: %add.r = fadd float %cf.real, 1.000000e+00
-// CHECK: %add.i = fadd float %cf.imag, 0.000000e+00
-// CHECK: store volatile float %add.r
-// CHECK: store volatile float %add.i, ptr getelementptr
+// CHECK: [[Re1:%.*]] = load volatile float, ptr @cf
+// CHECK: [[Im1:%.*]] = load volatile float, ptr getelementptr
+// CHECK: [[Add1:%.*]] = fadd float [[Re1]], 1.000000e+00
+// CHECK: store volatile float [[Add1]], ptr @cf
+// CHECK: store volatile float [[Im1]], ptr getelementptr
static_cast<volatile _Complex float &>(cf) = static_cast<volatile _Complex float&>(cf) + 1;
-// CHECK: %cf.real1 = load volatile float, ptr @cf
-// CHECK: %cf.imag2 = load volatile float, ptr getelementptr
-// CHECK: %add.r3 = fadd float %cf.real1, 1.000000e+00
-// CHECK: %add.i4 = fadd float %cf.imag2, 0.000000e+00
-// CHECK: store volatile float %add.r3, ptr @cf
-// CHECK: store volatile float %add.i4, ptr getelementptr
+// CHECK: [[Re2:%.*]] = load volatile float, ptr @cf
+// CHECK: [[Im2:%.*]] = load volatile float, ptr getelementptr
+// CHECK: [[Add2:%.*]] = fadd float [[Re2]], 1.000000e+00
+// CHECK: store volatile float [[Add2]], ptr @cf
+// CHECK: store volatile float [[Im2]], ptr getelementptr
const_cast<volatile int &>(a.a) = const_cast<volatile int &>(t.a) ;
-// CHECK: %0 = load volatile i32, ptr @t
-// CHECK: store volatile i32 %0, ptr @a
+// CHECK: [[I1:%.*]] = load volatile i32, ptr @t
+// CHECK: store volatile i32 [[I1]], ptr @a
static_cast<volatile int &>(a.b) = static_cast<volatile int &>(t.a) ;
-// CHECK: %1 = load volatile i32, ptr @t
-// CHECK: store volatile i32 %1, ptr getelementptr
+// CHECK: [[I2:%.*]] = load volatile i32, ptr @t
+// CHECK: store volatile i32 [[I2]], ptr getelementptr
const_cast<volatile int&>(vt) = const_cast<volatile int&>(vt) + 1;
-// CHECK: %2 = load volatile i32, ptr @vt
-// CHECK: %add = add nsw i32 %2, 1
-// CHECK: store volatile i32 %add, ptr @vt
+// CHECK: [[I3:%.*]] = load volatile i32, ptr @vt
+// CHECK: [[Add3:%.*]] = add nsw i32 [[I3]], 1
+// CHECK: store volatile i32 [[Add3]], ptr @vt
static_cast<volatile int&>(vt) = static_cast<volatile int&>(vt) + 1;
-// CHECK: %3 = load volatile i32, ptr @vt
-// CHECK: %add5 = add nsw i32 %3, 1
-// CHECK: store volatile i32 %add5, ptr @vt
+// CHECK: [[I4:%.*]] = load volatile i32, ptr @vt
+// CHECK: [[Add4:%.*]] = add nsw i32 [[I4]], 1
+// CHECK: store volatile i32 [[Add4]], ptr @vt
vt = const_cast<int&>(vol);
-// %4 = load i32, ptr @vol
-// store i32 %4, ptr @vt
+// [[I5:%.*]] = load i32, ptr @vol
+// store i32 [[I5]], ptr @vt
}
diff --git a/clang/test/Sema/complex-arithmetic.c b/clang/test/Sema/complex-arithmetic.c
new file mode 100644
index 00000000000000..c9e84da6daa9dc
--- /dev/null
+++ b/clang/test/Sema/complex-arithmetic.c
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 -verify %s
+// expected-no-diagnostics
+
+// This tests evaluation of _Complex arithmetic at compile time.
+
+#define APPROX_EQ(a, b) ( \
+ __builtin_fabs(__real (a) - __real (b)) < 0.0001 && \
+ __builtin_fabs(__imag (a) - __imag (b)) < 0.0001 \
+)
+
+#define EVAL(a, b) _Static_assert(a == b, "")
+#define EVALF(a, b) _Static_assert(APPROX_EQ(a, b), "")
+
+// _Complex float + _Complex float
+void a() {
+ EVALF((2.f + 3i) + (4.f + 5i), 6.f + 8i);
+ EVALF((2.f + 3i) - (4.f + 5i), -2.f - 2i);
+ EVALF((2.f + 3i) * (4.f + 5i), -7.f + 22i);
+ EVALF((2.f + 3i) / (4.f + 5i), 0.5609f + 0.0487i);
+
+ EVALF((2. + 3i) + (4. + 5i), 6. + 8i);
+ EVALF((2. + 3i) - (4. + 5i), -2. - 2i);
+ EVALF((2. + 3i) * (4. + 5i), -7. + 22i);
+ EVALF((2. + 3i) / (4. + 5i), .5609 + .0487i);
+}
+
+// _Complex int + _Complex int
+void b() {
+ EVAL((2 + 3i) + (4 + 5i), 6 + 8i);
+ EVAL((2 + 3i) - (4 + 5i), -2 - 2i);
+ EVAL((2 + 3i) * (4 + 5i), -7 + 22i);
+ EVAL((8 + 30i) / (4 + 5i), 4 + 1i);
+}
+
+// _Complex float + float
+void c() {
+ EVALF((2.f + 4i) + 3.f, 5.f + 4i);
+ EVALF((2.f + 4i) - 3.f, -1.f + 4i);
+ EVALF((2.f + 4i) * 3.f, 6.f + 12i);
+ EVALF((2.f + 4i) / 2.f, 1.f + 2i);
+
+ EVALF(3.f + (2.f + 4i), 5.f + 4i);
+ EVALF(3.f - (2.f + 4i), 1.f - 4i);
+ EVALF(3.f * (2.f + 4i), 6.f + 12i);
+ EVALF(3.f / (2.f + 4i), .3f - 0.6i);
+
+ EVALF((2. + 4i) + 3., 5. + 4i);
+ EVALF((2. + 4i) - 3., -1. + 4i);
+ EVALF((2. + 4i) * 3., 6. + 12i);
+ EVALF((2. + 4i) / 2., 1. + 2i);
+
+ EVALF(3. + (2. + 4i), 5. + 4i);
+ EVALF(3. - (2. + 4i), 1. - 4i);
+ EVALF(3. * (2. + 4i), 6. + 12i);
+ EVALF(3. / (2. + 4i), .3 - 0.6i);
+}
+
+// _Complex int + int
+void d() {
+ EVAL((2 + 4i) + 3, 5 + 4i);
+ EVAL((2 + 4i) - 3, -1 + 4i);
+ EVAL((2 + 4i) * 3, 6 + 12i);
+ EVAL((2 + 4i) / 2, 1 + 2i);
+
+ EVAL(3 + (2 + 4i), 5 + 4i);
+ EVAL(3 - (2 + 4i), 1 - 4i);
+ EVAL(3 * (2 + 4i), 6 + 12i);
+ EVAL(20 / (2 + 4i), 2 - 4i);
+}
+
+// _Complex float + int
+void e() {
+ EVALF((2.f + 4i) + 3, 5.f + 4i);
+ EVALF((2.f + 4i) - 3, -1.f + 4i);
+ EVALF((2.f + 4i) * 3, 6.f + 12i);
+ EVALF((2.f + 4i) / 2, 1.f + 2i);
+
+ EVALF(3 + (2.f + 4i), 5.f + 4i);
+ EVALF(3 - (2.f + 4i), 1.f - 4i);
+ EVALF(3 * (2.f + 4i), 6.f + 12i);
+ EVALF(3 / (2.f + 4i), .3f - 0.6i);
+
+ EVALF((2. + 4i) + 3, 5. + 4i);
+ EVALF((2. + 4i) - 3, -1. + 4i);
+ EVALF((2. + 4i) * 3, 6. + 12i);
+ EVALF((2. + 4i) / 2, 1. + 2i);
+
+ EVALF(3 + (2. + 4i), 5. + 4i);
+ EVALF(3 - (2. + 4i), 1. - 4i);
+ EVALF(3 * (2. + 4i), 6. + 12i);
+ EVALF(3 / (2. + 4i), .3 - 0.6i);
+}
+
+// _Complex int + float
+void f() {
+ EVALF((2 + 4i) + 3.f, 5.f + 4i);
+ EVALF((2 + 4i) - 3.f, -1.f + 4i);
+ EVALF((2 + 4i) * 3.f, 6.f + 12i);
+ EVALF((2 + 4i) / 2.f, 1.f + 2i);
+
+ EVALF(3.f + (2 + 4i), 5.f + 4i);
+ EVALF(3.f - (2 + 4i), 1.f - 4i);
+ EVALF(3.f * (2 + 4i), 6.f + 12i);
+ EVALF(3.f / (2 + 4i), .3f - 0.6i);
+
+ EVALF((2 + 4i) + 3., 5. + 4i);
+ EVALF((2 + 4i) - 3., -1. + 4i);
+ EVALF((2 + 4i) * 3., 6. + 12i);
+ EVALF((2 + 4i) / 2., 1. + 2i);
+
+ EVALF(3. + (2 + 4i), 5. + 4i);
+ EVALF(3. - (2 + 4i), 1. - 4i);
+ EVALF(3. * (2 + 4i), 6. + 12i);
+ EVALF(3. / (2 + 4i), .3 - 0.6i);
+}
|
Sirraide
commented
Feb 26, 2024
|
Since this is a relatively benign bug, all things considered, does this even warrant a release note? |
|
From what I can tell, this also fixes #31205 |
I think it should have a release note; it changes the layout of the AST which may impact folks writing AST matchers (clang-tidy checks, etc). But also, I think it does fix #31205 which is worth mentioning as users can notice the performance difference on multiplication and division (not so much on addition or subtraction, I believe). I think the code is correct and the extra test coverage is awesome, but I've added @jcranmer-intel as a reviewer to double check that everything is correct. |
Yeah, I wrote that before I noticed that there was an open issue about it iirc; I’ll add a release note for this. |
|
@jcranmer-intel @AaronBallman Do you think there’s anything else that needs to be done here, or is it fine as-is? |
|
Alright, just checking that the AST dump doesn’t contain |
AaronBallman
approved these changes
Mar 13, 2024
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.
C23 6.3.1.8 ‘Usual arithmetic conversions’ p1 states (emphasis mine):
‘type domain’ here refers to
_Complexvs real (i.e. non-_Complex); there is another clause that states the same fordouble.Consider the following code:
After talking this over with @AaronBallman, we came to the conclusion that
xshould be converted tofloatand not_Complex float(that is, we should perform a division of_Complex float / float, and not_Complex float / _Complex float; the same also applies to-+*). This was already being done correctly for cases wherexwas already afloat; it’s just mixed_Complex float+intoperations that currently suffer from this problem.This pr removes the extra
FloatingRealToComplexconversion that we were erroneously inserting and adds some tests to make sure we’re actually doing_Complex float / floatand not_Complex float / _Complex float(and analogously fordoubleand-+*).The only exception here is
float / _Complex float, which calls a library function (__divsc3) that takes 4floats, so we end up having to convert thefloatto a_Complex floatafter all (and analogously fordouble); I don’t believe there is a way around this.Lastly, we were also missing tests for
_Complexarithmetic at compile time, from what I can tell, so I’ve added some tests for that as well.