Skip to content
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

[C99] Claim conformance for _Complex support #88161

Merged
merged 12 commits into from
Apr 12, 2024

Conversation

AaronBallman
Copy link
Collaborator

There's so much overlap between the cited papers so this condenses the status page into a single entry rather than trying to test conformance against multiple papers doing conflicting things.

There's so much overlap between the cited papers so this condenses the
status page into a single entry rather than trying to test conformance
against multiple papers doing conflicting things.
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Apr 9, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

There's so much overlap between the cited papers so this condenses the status page into a single entry rather than trying to test conformance against multiple papers doing conflicting things.


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

2 Files Affected:

  • (added) clang/test/C/C99/n809.c (+97)
  • (modified) clang/www/c_status.html (+8-22)
diff --git a/clang/test/C/C99/n809.c b/clang/test/C/C99/n809.c
new file mode 100644
index 00000000000000..8d26744443eec4
--- /dev/null
+++ b/clang/test/C/C99/n809.c
@@ -0,0 +1,97 @@
+// RUN: %clang_cc1 -verify -std=c99 %s
+
+/* WG14 N620, N638, N657, N694, N809: Yes*
+ * Complex and imaginary support in <complex.h>
+ *
+ * NB: Clang supports _Complex but not _Imaginary. In C99, _Complex support is
+ * required outside of freestanding, but _Imaginary support is fully optional.
+ * In C11, both are made fully optional. We claim full conformance because we
+ * are actually conforming, but this gets an asterisk because it's also only
+ * partially implemented in a way and users should know about that.
+ *
+ * Because the functionality is so intertwined between the various papers,
+ * we're testing all of the functionality in one file.
+ */
+
+// Demonstrate that we support spelling complex floating-point objects.
+float _Complex f1;
+_Complex float f2;
+
+double _Complex d1;
+_Complex double d2;
+
+long double _Complex ld1;
+_Complex long double ld2;
+
+// Show that we don't support spelling imaginary types.
+float _Imaginary fi1; // expected-error {{imaginary types are not supported}}
+_Imaginary float fi2; // expected-error {{imaginary types are not supported}}
+
+double _Imaginary di1; // expected-error {{imaginary types are not supported}}
+_Imaginary double di2; // expected-error {{imaginary types are not supported}}
+
+long double _Imaginary ldi1; // expected-error {{imaginary types are not supported}}
+_Imaginary long double ldi2; // expected-error {{imaginary types are not supported}}
+
+// Each complex type has the same representation and alignment as an array
+// containing two elements of the corresponding real type.
+_Static_assert(sizeof(float _Complex) == sizeof(struct { float mem[2]; }), "");
+_Static_assert(_Alignof(float _Complex) == _Alignof(struct { float mem[2]; }), "");
+
+_Static_assert(sizeof(double _Complex) == sizeof(struct { double mem[2]; }), "");
+_Static_assert(_Alignof(double _Complex) == _Alignof(struct { double mem[2]; }), "");
+
+_Static_assert(sizeof(long double _Complex) == sizeof(struct { long double mem[2]; }), "");
+_Static_assert(_Alignof(long double _Complex) == _Alignof(struct { long double mem[2]; }), "");
+
+// The first element corresponds to the real part and the second element
+// corresponds to the imaginary part.
+_Static_assert(__real((float _Complex){ 1.0f, 2.0f }) == 1.0f, "");
+_Static_assert(__imag((float _Complex){ 1.0f, 2.0f }) == 2.0f, "");
+
+_Static_assert(__real((double _Complex){ 1.0, 2.0 }) == 1.0, "");
+_Static_assert(__imag((double _Complex){ 1.0, 2.0 }) == 2.0, "");
+
+_Static_assert(__real((long double _Complex){ 1.0L, 2.0L }) == 1.0L, "");
+_Static_assert(__imag((long double _Complex){ 1.0L, 2.0L }) == 2.0L, "");
+
+// When a real value is converted to a complex value, the real part follows the
+// usual conversion rules and the imaginary part should be zero.
+_Static_assert(__real((float _Complex)1.0f) == 1.0f, "");
+_Static_assert(__imag((float _Complex)1.0f) == 0.0f, "");
+
+_Static_assert(__real((double _Complex)1.0f) == 1.0, "");
+_Static_assert(__imag((double _Complex)1.0f) == 0.0, "");
+
+_Static_assert(__real((long double _Complex)1.0f) == 1.0L, "");
+_Static_assert(__imag((long double _Complex)1.0f) == 0.0L, "");
+
+// When a complex value is converted to a real value, the real part follows the
+// usual conversion rules and the imaginary part is discarded.
+_Static_assert((float)(float _Complex){ 1.0f, 2.0f } == 1.0f, "");
+_Static_assert((double)(float _Complex){ 1.0f, 2.0f } == 1.0, "");
+_Static_assert((long double)(float _Complex){ 1.0f, 2.0f } == 1.0L, "");
+
+// Complex values are only equal if both the real and imaginary parts are equal.
+_Static_assert((float _Complex){ 1.0f, 2.0f } == (float _Complex){ 1.0f, 2.0f }, "");
+_Static_assert((double _Complex){ 1.0, 2.0 } == (double _Complex){ 1.0, 2.0 }, "");
+_Static_assert((long double _Complex){ 1.0L, 2.0L } == (long double _Complex){ 1.0L, 2.0L }, "");
+
+_Static_assert((float _Complex){ 1.0f, 2.0f } != (float _Complex){ 2.0f, 0.0f }, "");
+_Static_assert((double _Complex){ 1.0, 2.0 } != (double _Complex){ 2.0, 0.0 }, "");
+_Static_assert((long double _Complex){ 1.0L, 2.0L } != (long double _Complex){ 2.0L, 0.0L }, "");
+
+// You cannot use relational operator on complex values.
+int i1 = (float _Complex){ 1.0f, 2.0f } < 10;        // expected-error {{invalid operands to binary expression}}
+int i2 = (double _Complex){ 1.0f, 2.0f } > 10;       // expected-error {{invalid operands to binary expression}}
+int i3 = (long double _Complex){ 1.0f, 2.0f } <= 10; // expected-error {{invalid operands to binary expression}}
+int i4 = (float _Complex){ 1.0f, 2.0f } >= 10;       // expected-error {{invalid operands to binary expression}}
+
+// As a type specifier, _Complex cannot appear alone; however, we support it as
+// an extension by assuming _Complex double.
+_Complex c = 1.0f; // expected-warning {{plain '_Complex' requires a type specifier; assuming '_Complex double'}}
+// Because we don't support imaginary types, we don't extend the extension to
+// that type specifier.
+// FIXME: the warning diagnostic here is incorrect and should not be emitted.
+_Imaginary i = 1.0f; // expected-warning {{plain '_Complex' requires a type specifier; assuming '_Complex double'}} \
+                        expected-error {{imaginary types are not supported}}
diff --git a/clang/www/c_status.html b/clang/www/c_status.html
index 7ee1d2b507e88c..99a65bfb0a48e5 100644
--- a/clang/www/c_status.html
+++ b/clang/www/c_status.html
@@ -132,29 +132,11 @@ <h2 id="c99">C99 implementation status</h2>
       <td>Unknown</td>
       <td class="full" align="center">Yes</td>
     </tr>
-    <tr id="complex">
-      <td rowspan="6">complex and imaginary support in &lt;complex.h&gt;</td>
+    <tr>
+      <td>complex and imaginary support in &lt;complex.h&gt;</td>
+      <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n693.ps">N693</a></td>
+      <td class="full" align="center">Yes <a href="#complex">(1)</a></td>
     </tr>
-      <tr>
-        <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n620.ps">N620</a></td>
-        <td class="unknown" align="center">Unknown</td>
-      </tr>
-      <tr>
-        <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n638.ps">N638</a></td>
-        <td class="unknown" align="center">Unknown</td>
-      </tr>
-      <tr>
-        <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n657.ps">N657</a></td>
-        <td class="unknown" align="center">Unknown</td>
-      </tr>
-      <tr>
-        <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n694.ps">N694</a></td>
-        <td class="unknown" align="center">Unknown</td>
-      </tr>
-      <tr>
-        <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n809.ps">N809</a></td>
-        <td class="unknown" align="center">Unknown</td>
-      </tr>
     <tr>
       <td>type-generic math macros in &lt;tgmath.h&gt;</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n693.ps">N693</a></td>
@@ -373,6 +355,10 @@ <h2 id="c99">C99 implementation status</h2>
       <td class="full" align="center">Yes</td>
     </tr>
 </table>
+<span id="complex">(1): Clang supports <code>_Complex</code> type specifiers but
+does not support <code>_Imaginary</code> type specifiers. Support for
+<code>_Imaginary</code> is optional in C99 which is why Clang is fully conforming.
+</span>
 </details>
 
 <h2 id="c11">C11 implementation status</h2>

@AaronBallman
Copy link
Collaborator Author

I'm not convinced I have sufficient tests, so I'm especially interested in some help figuring out what else to test and how to go about it. I don't believe we need to test library behavior here because Clang does not provide any library support for complex types in freestanding mode; we expect the user to link against a C standard library that has such support.

@zahiraam
Copy link
Contributor

zahiraam commented Apr 9, 2024

I'm not convinced I have sufficient tests, so I'm especially interested in some help figuring out what else to test and how to go about it. I don't believe we need to test library behavior here because Clang does not provide any library support for complex types in freestanding mode; we expect the user to link against a C standard library that has such support.

I haven't looked at it in details yet, but the test needs to run on Linux and Windows.

@AaronBallman
Copy link
Collaborator Author

I'm not convinced I have sufficient tests, so I'm especially interested in some help figuring out what else to test and how to go about it. I don't believe we need to test library behavior here because Clang does not provide any library support for complex types in freestanding mode; we expect the user to link against a C standard library that has such support.

I haven't looked at it in details yet, but the test needs to run on Linux and Windows.

I don't think there's anything platform-specific about our support at this level, so the test has no triples in the RUN line (so it will be run on all triples we test on which includes Linux and Windows). Or do you mean something else?

There's already a (1) in the status file, so this reduces confusion.
@zahiraam
Copy link
Contributor

zahiraam commented Apr 9, 2024

I'm not convinced I have sufficient tests, so I'm especially interested in some help figuring out what else to test and how to go about it. I don't believe we need to test library behavior here because Clang does not provide any library support for complex types in freestanding mode; we expect the user to link against a C standard library that has such support.

I haven't looked at it in details yet, but the test needs to run on Linux and Windows.

I don't think there's anything platform-specific about our support at this level, so the test has no triples in the RUN line (so it will be run on all triples we test on which includes Linux and Windows). Or do you mean something else?

I guess at the IR level no. It's just that I have recently found that this test:
#include <stdio.h>
#include <complex.h>

int main()
{
float complex b = -8.45345913e+29f + -1.23588801e+31f * I;
float complex a = 1.25284138e+31f + -7.91789592e+30f * I;
float complex c = a / b;
printf("%f %f\n", creal(c), cimag(c));
return 0;
}
But fails on Windows.

@frederick-vs-ja
Copy link
Contributor

But fails on Windows.

This seems because of that MS UCRT's complex math functions are non-conforming.

I guess they could work if we write the following before including UCRT's <complex.h>.

#define _C_COMPLEX_T
typedef double _Complex _C_double_complex;
typedef float _Complex _C_float_complex;
typedef long double _Complex _C_ldouble_complex;

@philnik777
Copy link
Contributor

Note that

auto div(_Complex float lhs, _Complex float rhs) {
  return lhs / rhs;
}

int main() {
  return __real div(1.f, 2.f);
}

will fail to link on windows due to __divsc3 not being available. Similar programs probably also fail due to other compiler-rt functions not being available.

@AaronBallman
Copy link
Collaborator Author

Note that

auto div(_Complex float lhs, _Complex float rhs) {
  return lhs / rhs;
}

int main() {
  return __real div(1.f, 2.f);
}

will fail to link on windows due to __divsc3 not being available. Similar programs probably also fail due to other compiler-rt functions not being available.

I think Clang still gets to claim conformance here because we handle the language side of things correctly and the user is responsible for linking to a runtime library with appropriate support for the platform. However, this sure does straddle the line in some ways, so I could see this being a reason to claim "partial" conformance.

Copy link
Contributor

@jcranmer-intel jcranmer-intel left a comment

Choose a reason for hiding this comment

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

You're missing checks for type domain rules, so things like:

  • converting between float _Complex and double _Complex
  • common type of float _Complex and double
  • result of int and float _Complex
  • complex types not allowed in increment/decrement operators
  • complex types permitted in unary +, -

Not sure how to test this, but _Complex float not promoting to _Complex double like float does to double is also worth slotting under complex conformance. Also 6.7.8p10 (static-storage duration arithmetic types (which include complex types) are initialized to 0).

clang/test/C/C99/n809.c Show resolved Hide resolved
@@ -373,6 +355,10 @@ <h2 id="c99">C99 implementation status</h2>
<td class="full" align="center">Yes</td>
</tr>
</table>
<span id="complex">(2): Clang supports <code>_Complex</code> type specifiers but
does not support <code>_Imaginary</code> type specifiers. Support for
<code>_Imaginary</code> is optional in C99 which is why Clang is fully conforming.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention here something about not claiming Annex G conformance?

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

Comment on lines +49 to +50
_Static_assert(__real((float _Complex){ 1.0f, 2.0f }) == 1.0f, "");
_Static_assert(__imag((float _Complex){ 1.0f, 2.0f }) == 2.0f, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using an extension for complex initializers, not a standard feature, so I'm a little bit uncomfortable that it's actually testing the assertion that the first element of the array corresponds to the real.

That said, I'm not sure there's any non-extension way to test that as a constant expression anyways, so... 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other alternative would be to use a builtin function, I can go either way. Maybe I should use one of each to show they're equivalent?

@AaronBallman
Copy link
Collaborator Author

You're missing checks for type domain rules, so things like:

* converting between `float _Complex` and `double _Complex`

* common type of `float _Complex` and `double`

* result of `int` and `float _Complex`

* complex types not allowed in increment/decrement operators

* complex types permitted in unary +, -

Thanks for the suggestions, these are now being tested.

Not sure how to test this, but _Complex float not promoting to _Complex double like float does to double is also worth slotting under complex conformance. Also 6.7.8p10 (static-storage duration arithmetic types (which include complex types) are initialized to 0).

I'll add tests for these as well.

@philnik777
Copy link
Contributor

Note that

auto div(_Complex float lhs, _Complex float rhs) {
  return lhs / rhs;
}

int main() {
  return __real div(1.f, 2.f);
}

will fail to link on windows due to __divsc3 not being available. Similar programs probably also fail due to other compiler-rt functions not being available.

I think Clang still gets to claim conformance here because we handle the language side of things correctly and the user is responsible for linking to a runtime library with appropriate support for the platform. However, this sure does straddle the line in some ways, so I could see this being a reason to claim "partial" conformance.

IIRC the problem is that you can't get compiler-rt on windows. If it was just a matter of compiling compiler-rt with clang I'd be 100% with you. Partial conformance with a note that some operations require compiler-rt functions that aren't available on windows seems like a good compromise to me.

@AaronBallman
Copy link
Collaborator Author

Note that

auto div(_Complex float lhs, _Complex float rhs) {
  return lhs / rhs;
}

int main() {
  return __real div(1.f, 2.f);
}

will fail to link on windows due to __divsc3 not being available. Similar programs probably also fail due to other compiler-rt functions not being available.

I think Clang still gets to claim conformance here because we handle the language side of things correctly and the user is responsible for linking to a runtime library with appropriate support for the platform. However, this sure does straddle the line in some ways, so I could see this being a reason to claim "partial" conformance.

IIRC the problem is that you can't get compiler-rt on windows. If it was just a matter of compiling compiler-rt with clang I'd be 100% with you. Partial conformance with a note that some operations require compiler-rt functions that aren't available on windows seems like a good compromise to me.

Yeah, that seems like a reasonable compromise. I'll make updates for that, thanks!

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

LGTM from my side. I'll leave the conformance test details to the folks who know what they're doing.

@zahiraam
Copy link
Contributor

LGTM. Thanks.

@AaronBallman AaronBallman merged commit 0318ce8 into llvm:main Apr 12, 2024
3 of 4 checks passed
@AaronBallman AaronBallman deleted the aballman-c99-complex branch April 12, 2024 18:15
@h-vetinari
Copy link
Contributor

h-vetinari commented Apr 13, 2024

I'm surprised about the "compiler-rt is not supported on windows" comment - in our distribution, we've been building compiler-rt on windows for years, and I don't remember significant issues.

It's true that __divsc3 is not available without compiler-rt (i.e. just relying on the msvc runtime libraries), but together with compiler-rt these things are generally possible? Note that a similar situation exists on osx-arm for example - in certain situations, linking compiler-rt is unavoidable.

bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
There's so much overlap between the cited papers so this condenses the
status page into a single entry rather than trying to test conformance
against multiple papers doing conflicting things.
@AaronBallman
Copy link
Collaborator Author

I'm surprised about the "compiler-rt is not supported on windows" comment - in our distribution, we've been building compiler-rt on windows for years, and I don't remember significant issues.

It's true that __divsc3 is not available without compiler-rt (i.e. just relying on the msvc runtime libraries), but together with compiler-rt these things are generally possible? Note that a similar situation exists on osx-arm for example - in certain situations, linking compiler-rt is unavoidable.

CC @petrhosek as compiler-rt CRT code owner -- is compiler-rt officially supported for Windows? The official documentation does not mention it under platform support: https://compiler-rt.llvm.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c99 clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants