This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use _Complex for multiplication and division of complex floating point types
Needs ReviewPublic

Authored by philnik on Jul 14 2023, 10:04 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

This significantly simplifies the implementation and improves the codegen. The only downside is that the accuracy can be marginally worse, but that is up to the compiler to decide with this change, which means is can be controlled by compiler flags.

Diff Detail

Event Timeline

philnik created this revision.Jul 14 2023, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 10:04 AM
philnik requested review of this revision.Jul 14 2023, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 10:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 540735.Jul 15 2023, 1:48 PM

Try to fix CI

philnik edited the summary of this revision. (Show Details)Jul 15 2023, 1:59 PM

Can you modify the CI file to run with a limited number of configurations, but enable all platform.

Also fix the non floating point test to be fixed.

That way I can get a better view of the impact of the changes.

libcxx/test/std/numerics/complex.number/complex.member.ops/divide_equal_complex.pass.cpp
28 ↗(On Diff #540735)

I would like to test with an epsion and not >=.
I think the values used in the test should give an exact result.

Please verify nan and inf etc are tested otherwise test whether they change.

I think this is the right approach to implement floating-point complex. We should let the compiler do the work whenever we can. If the result is "not good enough", then that's something we should take to the compiler folks (probably compiler-rt) instead.

Also, this is how libstdc++ implements their std::complex operations, so it's not like we're doing something crazy here. The need to change tests makes me a tiny bit uncomfortable, but in reality I think the tests should have been written that way from the start -- comparing floating point values for equality is a well known anti pattern.

So this roughly looks good to me but I'd like to see again after updating with the comments and green CI.

libcxx/test/std/numerics/complex.number/complex.member.ops/divide_equal_complex.pass.cpp
28 ↗(On Diff #540735)

Yeah we should define a function to describe the fact that the value is "equal" (or "within the tolerance") instead of explicitly checking >= and <=.

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.cpp
15 ↗(On Diff #540735)

Is this still required? Applies here and in the other complex tests I guess.

philnik updated this revision to Diff 556803.Sep 14 2023, 12:15 PM
philnik marked 3 inline comments as done.

Address comments

philnik updated this revision to Diff 556960.Sep 18 2023, 10:03 AM

Try to fix CI

@mstorsjo Do you know what's the state of things for compiler-rt on Windows?

To summarize the issue, it seems like we are missing __muldc3 and __mulsc3 on Windows, which should be provided in compiler-rt (from https://buildkite.com/llvm-project/libcxx-ci/builds/29818#018aa944-7c19-4abf-8476-4b506918c33a).

@mstorsjo Do you know what's the state of things for compiler-rt on Windows?

In MinGW configs, compiler-rt is alive and well and everything works - and those configs work fine in that CI run.

To summarize the issue, it seems like we are missing __muldc3 and __mulsc3 on Windows, which should be provided in compiler-rt (from https://buildkite.com/llvm-project/libcxx-ci/builds/29818#018aa944-7c19-4abf-8476-4b506918c33a).

Indeed; in clang-cl configs, compiler-rt builtins aren't linked. This is the same issue as the helpers for __int128_t that also are missing that we've run into in a few places. There were some efforts to work on this (with some discussion threads on Discourse in the last few months) with @thieta, @rnk, @hans and others.