[flang] Replace uses of _Complex with std::complex

[echristo]

Summary

Fixing warning: '_Complex' is a C99 extension [-Wc99-extensions].

The functions are currently unused so no real testing has happened past build time, however, complex is strictly compatible with _Complex in the C++ standard and can be reinterpret casted in uses.

[DavidTruby]

LGTM, @schweitz might want to check in case there was a specific reason he was using _Complex previously.

[schweitz]

This will not compile in our builds.

[DavidTruby]

In D83397#2139078, @schweitz wrote:

This will not compile in our builds.

Could you give a little more info here? What's the error?

[echristo]

In whose builds and why? What changes would you like? What compiler are you using? Can you provide any more information?

[schweitz]

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

That code base is being upstreamed piecemeal. Not all of the code is upstreamed at this point. It is simply a false impression that code in the middle of being upstreamed is "unused" or "unnecessary". Since it not all of it is upstreamed, changing interfaces and support code in llvm-project directly is going to cause problems that can become hard to track and resolve while the upstreaming is ongoing.

Besides, there are plenty of compilation warnings in LLVM today, and many of them from personal experience have persisted for months before being resolved. This is not a unique case.

Is it possible to wait for the author to implement a solution? Or if not, can we just disable the warning?

[echristo]

In D83397#2139111, @schweitz wrote:

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

That's not part of the llvm project.

That code base is being upstreamed piecemeal. Not all of the code is upstreamed at this point. It is simply a false impression that code in the middle of being upstreamed is "unused" or "unnecessary". Since it not all of it is upstreamed, changing interfaces and support code in llvm-project directly is going to cause problems that can become hard to track and resolve while the upstreaming is ongoing.

It very much is unused and unnecessary as there are no pieces of that code in the repository.

Besides, there are plenty of compilation warnings in LLVM today, and many of them from personal experience have persisted for months before being resolved. This is not a unique case.

In general, I'd raise exception with "there are plenty of compilation warnings in LLVM today" as it's just not true - at least not with clang as I run a full build with Werror and fix any that slip in. Happy to have anything you're seeing fixed, but warnings with errors turned on are even on bots. Do you have a bot that shows a warning that's erroring out? This is the only warning that shows up in my full build with clang.

Is it possible to wait for the author to implement a solution? Or if not, can we just disable the warning?

What solution? What compiler is causing issues? If it's a supported compiler I'm happy to help figure it out, but this has been going on for a bit and it's a pair of one line fixes.

-eric

[schweitz]

In D83397#2139131, @echristo wrote:

In D83397#2139111, @schweitz wrote:

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

That's not part of the llvm project.

That code base is being upstreamed piecemeal. Not all of the code is upstreamed at this point. It is simply a false impression that code in the middle of being upstreamed is "unused" or "unnecessary". Since it not all of it is upstreamed, changing interfaces and support code in llvm-project directly is going to cause problems that can become hard to track and resolve while the upstreaming is ongoing.

It very much is unused and unnecessary as there are no pieces of that code in the repository.

We are stuck between a rock and a hard place. Flang needs a middle end, as it currently can't generate code. Work on that middle end is being done. We are following the rules and trying to upstream that code in small "reviewable" chunks as quickly as possible. It's just simply going to be the case because of the interdependencies involved and the small chunks process that some code will merely appear to be temporally unused.

If you have a better solution to how to change the upstream process, then our group would be happy to hear it.

[echristo]

In D83397#2139187, @schweitz wrote:

In D83397#2139131, @echristo wrote:

In D83397#2139111, @schweitz wrote:

Hi Eric,

There is an active development branch for the flang middle end. https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev

That's not part of the llvm project.

That code base is being upstreamed piecemeal. Not all of the code is upstreamed at this point. It is simply a false impression that code in the middle of being upstreamed is "unused" or "unnecessary". Since it not all of it is upstreamed, changing interfaces and support code in llvm-project directly is going to cause problems that can become hard to track and resolve while the upstreaming is ongoing.

It very much is unused and unnecessary as there are no pieces of that code in the repository.

We are stuck between a rock and a hard place. Flang needs a middle end, as it currently can't generate code. Work on that middle end is being done. We are following the rules and trying to upstream that code in small "reviewable" chunks as quickly as possible. It's just simply going to be the case because of the interdependencies involved and the small chunks process that some code will merely appear to be temporally unused.

If you have a better solution to how to change the upstream process, then our group would be happy to hear it.

You commit something as you get a use for it. Partial files etc. At least this is what the rest of us have done. :)

You also still haven't replied with "what bot/compiler/etc is going to break by making this change". Would you please do that?

Thanks.

[schweitz]

I've reverted these models from the upstreamed code for now. It will avoid the warnings.

[schweitz]

In D83397#2139202, @echristo wrote:

You also still haven't replied with "what bot/compiler/etc is going to break by making this change". Would you please do that?

Locally, we build flang with our changes with g++ and clang++ on a variety of Linux and MacOS.

[echristo]

In D83397#2139319, @schweitz wrote:

I've reverted these models from the upstreamed code for now. It will avoid the warnings.

OK, thanks! There are a few more unused ones too, but that's not a warning that's currently enabled for flang (should be eventually of course).

[echristo]

In D83397#2139336, @schweitz wrote:

In D83397#2139202, @echristo wrote:

You also still haven't replied with "what bot/compiler/etc is going to break by making this change". Would you please do that?

Locally, we build flang with our changes with g++ and clang++ on a variety of Linux and MacOS.

So do I as do a bunch of the builders so I'm still unsure what issues you're going to see on the change? Can you elaborate?

[DavidTruby]

I would actually say this is more serious than just a warning (although clang only has it as a warning for various reasons).
_Complex is not a keyword that exists in C++, so this code is non-conformant. It also isn't implemented in MSVC which is a compiler we should try to support as the rest of LLVM does.
It's also just less ergonomic to use than std::complex and they are guaranteed to be layout compatible, so if we need to call into/out of C the following still works:

// file.cpp
#include <complex>
extern "C" float real_part(std::complex<float> c) {
  return c.real();
}

// file.c
#include <stdio.h>
#include <complex.h>
extern float real_part(float _Complex c);

int main()
{
  float _Complex c = 1+2*I;
  printf("%f\n", real_part(c));
}

This isn't accidental; the C++ standard guarantees that this works.

[jeanPerier]

Sorry I was harvesting potatoes far from any computers when all these discussions happened. First of all, thanks for detecting the warning and proposing a fix for it. I will try to explain our issue and why we cannot use safely std::complex here so we can find a fix for this.

1. We want to have the possibility to target the pgmath runtime library that is open sourced here: https://github.com/flang-compiler/flang/tree/master/runtime/libpgmath. It is written in C99 and uses _Complex at the interface of its complex functions (you are absolutely right to call out windows issues here, pgmath actually uses _Fcomplex there, and we should at least align in that case to help supporting windows support in flang).
2. We want to both be able to generate code calling this runtime and be able to call this very runtime at compile time while folding for best numerical stability (at least when host and target are the same). So we need flang to safely link with pgmath on top of emitting code for it (You can actually already do that if you have a pgmath library under the hands by using -DLIBPGMATH_DIR=<path to pgmath lib> in your flang cmake command. This cmake flag is only advertised in flang/documentation/Intrinsics.md so far).
3. _Complex and std::complex are indeed layout compatible, but they are not guaranteed to be compatible in call interface. std::complex<> is an aggregate type that is opaque to the compiler, _Complex is a fundamental type for which some ABIs have different interface requirements. So we cannot declare pgmath functions in flang with std::complex. More justification below.

Proposal fix:
My proposal is therefore to follow libpgmath handling and define float_complex_t/ double_complex_t alias that will be _Complex on platform that supports it (disabling the warning in such cases) and _Fcomplex/_Dcomplex on windows (@echristo, I am not aware of compilers other than MSVC that do not support _Complex (but I honestly have little experience outside of gcc/clang) do you know of any ? I am not sure what to do in case _Complex is not supported and this is not MSVC. The solution I see would be to abort compilation if such platform/compiler exists and add correct _Complex replacement in this context.

Does that seems right to you ?

Justification of 3. :

@DavidTruby, your example happens to work on all architecture I know of but is not defined by the C++ standard (so that is a happy accident ;) ). As documented in this comment hidden in flang code: https://github.com/llvm/llvm-project/blob/master/flang/lib/Evaluate/intrinsics-library.cpp#L239, had you written a function returning a complex by value, I could actually have pointed at least X86 as an architecture where your example would crash or return an incorrect value. Many of the pgmath functions are returning complex values.

You do not have to trust me on this, you can just try throwing the following code to clang 10:

#include <complex>
extern "C" std::complex<float> conjugate(std::complex<float>);

You will get warning: 'conjugate' has C-linkage specified, but returns user-defined type 'std::complex<float>' which is incompatible with C [-Wreturn-type-c-linkage].
That is because linkage and layout compatibility are not the same thing. Layout compatibility can only be brought-up when things are in memory, but to my knowledge, nothing in the C99 standard says that _Complex have to be passed/returned in memory, this choice is left to the target architecture ABIs. For instance, if you have a look at the System V i386 ABI (https://uclibc.org/docs/psABI-i386.pdf) page 14, table 2.4, you will read regarding float _Complex : "The real part is returned in %eax. The imaginary part is returned in %edx.". However, std::complex<float> which is a user defined class (from the compiler point of view, STL types are not very different from user defined types) must be returned in memory (page 12: "Aggregate types (structs and unions) are always returned in memory.").
You can check on https://godbolt.org/ that the following program in with x86-64 clang 10 and -O2 -m32 flags yields different (incompatible) assembly for foo_cpp and foo_c99:

#include <complex>
extern "C" std::complex<float> foo_cpp(std::complex<float> x) {
 return x;
}
extern "C" float _Complex foo_c99(float _Complex x) {
 return x;
}

output:

foo_cpp:                                # @foo_cpp
        movl    4(%esp), %eax
        movsd   8(%esp), %xmm0          # xmm0 = mem[0],zero
        movsd   %xmm0, (%eax)
        retl    $4
foo_c99:                                # @foo_c99
        movl    4(%esp), %eax
        movl    8(%esp), %edx
        retl

[DavidTruby]

Thanks for the additional context; I checked returning std::complex on x86_64 and Aarch64 before my comment and it worked fine; I guess the calling convention for x86_32 differs here in a way I didn't appreciate.
At least on those two architectures, at each optimisation level the way the value is returned matches: at O0 and O1 both return in memory and at O2 and O3 both return in registers.

In this specific code, I don't see how the differing calling convention can be an issue though. These functions are not callable from C as they are template functions, so surely there's no need to match the C calling convention here?

In terms of a more general solution, I'm uncomfortable with the idea of simply aborting compilation in perfectly compliant C++ compilers due to the use of non-standard features. Would it be possible to write a function on the C side that wraps these functions and returns a struct with two floats/doubles (that will also be layout compatible with both types) to force the calling convention to match the C++ one? Those functions could then be called safely from C++ with std::complex prototypes.

[jeanPerier]

In this very spot, the goal of the template is to digest the runtime headers in order to lower their function signature to an mlir::FuncOp declaration that is compatible with the runtime and will be call in user programs.
So if we use std::complex here and introduce a wrapper indirection in C around the runtime, then the cost of this indirection will be paid by the user program. I am not comfortable with having a Fortran programmer pay the price of a C/C++ compatibility issues.

However, you are absolutely correct that in this very code spot we are discussing, using _Complex is not essential. All we need to know "this runtime interface is using C99 complex", so we could define a type like Fortran::common::C99complex<T> instead, and use it as a tag here.

In folding the the C99 _Complex functions are called from C++ the code around the comment I linked above is dealing with this interface (or was, since part of it was reverted). Your wrappers would be a solution here given folding performance is not as critical, but given folding with pgmath is the non-default option (under ifdef, I think it is easier to only enable it if _Complex of _FComplex is available and resort to the default usage of cmath in the other cases.

Thanks for pointing out we do not really use the interface in this very spot ! I think I can now put up a solution that should fulfill all wishes:

• no warnings, compiles on all C++ compliant compiler.
• no silent undefined behavior for X86_32 and the whatever other ABI out there deals with _Complex in a special way
• no user program performance hit
• still possible to fold with pgmath if you are using a mainstream C++ compiler to compile flang (g++, clang++, msvc when possible)
• no hard error if using pgmath complex functions is not possible
• not too much additional code complexity in flang or the runtime.

flang/lib/Lower/RTBuilder.h

 #ifndef FORTRAN_LOWER_RTBUILDER_H
 #define FORTRAN_LOWER_RTBUILDER_H
 
 #include "flang/Lower/ConvertType.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/IR/StandardTypes.h"
 #include "llvm/ADT/SmallVector.h"
+#include <complex>
 #include <functional>
 
 // List the runtime headers we want to be able to dissect
 #include "../../runtime/io-api.h"
 
 namespace Fortran::lower {
 
 using TypeBuilderFunc = mlir::Type (*)(mlir::MLIRContext *);

 constexpr TypeBuilderFunc getModel<bool &>() {
   return [](mlir::MLIRContext *context) -> mlir::Type {
     TypeBuilderFunc f{getModel<bool>()};
     return fir::ReferenceType::get(f(context));
   };
 }
 
 template <>
-constexpr TypeBuilderFunc getModel<float _Complex>() {
+constexpr TypeBuilderFunc getModel<std::complex<float>>() {
   return [](mlir::MLIRContext *context) -> mlir::Type {
     return fir::CplxType::get(context, sizeof(float));
   };
 }
 template <>
-constexpr TypeBuilderFunc getModel<double _Complex>() {
+constexpr TypeBuilderFunc getModel<std::complex<double>>() {
   return [](mlir::MLIRContext *context) -> mlir::Type {
     return fir::CplxType::get(context, sizeof(double));
   };
 }
 
 template <>
 constexpr TypeBuilderFunc getModel<const Fortran::runtime::Descriptor &>() {
   return [](mlir::MLIRContext *context) -> mlir::Type {