This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add cpowi function to runtime and use instead of pgmath
ClosedPublic

Authored by DavidTruby on Sep 29 2022, 9:11 AM.

Details

Summary

This patch adds a cpowi function to the flang runtime, and switches
to using that function instead of pgmath for complex number to
integer power operations.

Diff Detail

Event Timeline

DavidTruby created this revision.Sep 29 2022, 9:11 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: mehdi_amini. · View Herald Transcript
DavidTruby requested review of this revision.Sep 29 2022, 9:11 AM

It looks like MSVC doesn't like what I have done here. Does anyone have any suggestions? I don't have access to a Windows install to test this myself...

It looks like MSVC doesn't like what I have done here. Does anyone have any suggestions? I don't have access to a Windows install to test this myself...

Try renaming complex-powi.cpp into complex-powi.c.

It looks like MSVC doesn't like what I have done here. Does anyone have any suggestions? I don't have access to a Windows install to test this myself...

Try renaming complex-powi.cpp into complex-powi.c.

In that case I'd have to rewrite it somehow as I'm using a template in there to abstract the 4 different types we need (as the implementation is identical)
I think it might be that #include complex.h is outside the ifdef for msvc?

@klausler, can you please have a look?

flang/include/flang/Runtime/complex-wrapper.h
5 ↗(On Diff #463931)

Please move the header comment to the beginning of the file.

flang/runtime/complex-powi.cpp
68

Please use int64_t instead of long here and below.

It looks like MSVC doesn't like what I have done here. Does anyone have any suggestions? I don't have access to a Windows install to test this myself...

Try renaming complex-powi.cpp into complex-powi.c.

In that case I'd have to rewrite it somehow as I'm using a template in there to abstract the 4 different types we need (as the implementation is identical)
I think it might be that #include complex.h is outside the ifdef for msvc?

I do not think the placement of #include <complex.h> matters. You will still have to include it for MSVC, and it is deprecated in C++17 as MSVC states.
You may try using the same adaptor technique as in complex-reduction.c.

I do not think the placement of #include <complex.h> matters. You will still have to include it for MSVC, and it is deprecated in C++17 as MSVC states.
You may try using the same adaptor technique as in complex-reduction.c.

Right, I've tried to move that adaptor here so that I could re-use it. But I think the #include <complex.h> needs to be inside the non-MSVC case and then we just use the MSVC builtins in that case.

flang/runtime/complex-powi.cpp
68

Should I also use int32_t as well? I guess so

I do not think the placement of #include <complex.h> matters. You will still have to include it for MSVC, and it is deprecated in C++17 as MSVC states.
You may try using the same adaptor technique as in complex-reduction.c.

Right, I've tried to move that adaptor here so that I could re-use it. But I think the #include <complex.h> needs to be inside the non-MSVC case and then we just use the MSVC builtins in that case.

I guess you can try this.

flang/runtime/complex-powi.cpp
68

It should not matter int or int32_t, so it is up to you :)

It looks like there's no way to get it working this way with MSVC, so I will have to change the file to C and not use a template for it. I will probably put the implementation inside a macro and instantiate that for the different types we have instead.

I'll need to do something about the GTEST tests still as those have to be written in C++. I'll probably expose a pointer interface from the C side for this and pass a pointer to a C++ complex in, as the types are at least layout compatible.

It looks like there's no way to get it working this way with MSVC, so I will have to change the file to C and not use a template for it. I will probably put the implementation inside a macro and instantiate that for the different types we have instead.

I'll need to do something about the GTEST tests still as those have to be written in C++. I'll probably expose a pointer interface from the C side for this and pass a pointer to a C++ complex in, as the types are at least layout compatible.

I just want to point out that complex-reduction.c proposes a different solution that still allows using C++ templates for the actual implementation but requires defining C wrappers for interfacing with the compiler. I guess this way you might be able to use the Cpp names to directly invoke the C++ implementation methods with C++ complex data type. I know it is not testing exactly what the compiler generates, but at least you will not have to rely on the types layout compatibility.

DavidTruby updated this revision to Diff 464652.Oct 3 2022, 4:57 AM

Doing it using C didn't work either, as MSVC doesn't support operators on complex
numbers in C.
I'm hopeful that after investigation what I've done here will work correctly, but
I haven't been able to test it on Windows so I'm uploading it to see if it passes
the pre-commit CI.

DavidTruby updated this revision to Diff 464655.Oct 3 2022, 5:15 AM

Extra fixes for MSVC.
This is my last try before I set up a Windows dev environment for myself. Sorry for
the spam!

I agree that this is probably the best algorithm to use, rather than casting to a more general cpow or converting to/from polar coordinates. Thanks for working around MSVC's poor implementation of complex.

flang/include/flang/Runtime/entry-names.h
27

why is RTNAME_STRINGIFY_ needed? can't this just be #x?

flang/runtime/complex-powi.cpp
18

Don't bother with guesses about fast-path cases. The only one needed for correctness is 0 and that can be an if.

31

We use modern braced initialization in the runtime for safety. bool invertResult{exp < 0}; would be more clear.

33

This will fail when exp is the most negative value.

126

Please terminate the source file with a newline.

flang/unittests/Runtime/Complex.cpp
51

Braces, please.

146

Omit the lower-case letter "l" suffixes; they look like digits, they're not necessary, and they depend implicitly on long being 64 bits, which it isn't everywhere. If you must have explicit int64_t values, use static_cast<>.

DavidTruby updated this revision to Diff 465396.Oct 5 2022, 7:50 AM

Use brace initialization for variables.
Handle case where exp == INT_MIN
Additional fixes for MSVC

I don't think the linux failure here is down to my patch?
The MSVC one is; it seems cpowi and cpowk are passing but not zpowi or zpowk. I have no idea why, I don't have access to MSVC to check properly atm

DavidTruby added inline comments.Oct 5 2022, 11:05 AM
flang/include/flang/Runtime/entry-names.h
27

I tried it without and it doesn't expand to the right thing. I will be honest and admit I have no idea why :)

I don't think the linux failure here is down to my patch?
The MSVC one is; it seems cpowi and cpowk are passing but not zpowi or zpowk. I have no idea why, I don't have access to MSVC to check properly atm

Hi David, I will try to reproduce it on Windows today and will let you know.

Hi David, I will try to reproduce it on Windows today and will let you know.

That would be great, thanks! I'm trying to get a Windows setup that I can test it on but it's a slow process getting everything sorted... And I don't immediately understand the error message in the CI

vzakhari added inline comments.Oct 5 2022, 1:46 PM
flang/runtime/complex-powi.cpp
87

Can you please clarify where the documentation says that their _[FDL]complex implementation is necessarily using the structure representation?

I do not really like that we now have two potentially different ways of passing "complex" to the runtime functions (one here and another for complex reduction functions). I would rather have it uniform, so, for example, try to keep using float_Complex_t typedefs defined in complex-reduction.h

flang/unittests/Runtime/Complex.cpp
34

typo here and below: float -> double

This causes the test failure on Windows.

DavidTruby added inline comments.Oct 5 2022, 1:50 PM
flang/runtime/complex-powi.cpp
87

It's here in the docs:

The compiler doesn't directly support a complex or _Complex keyword, therefore the Microsoft implementation uses structure types to represent complex numbers.

I agree I'd rather use the stuff from complex-reduction.h and I tried that initially, but you cannot get _[FDL]complex in a C++ file without MSVC erroring out... This is the best way I found to work around those issues.

flang/unittests/Runtime/Complex.cpp
34

I assumed it would be a daft mistake. Thanks!

DavidTruby added inline comments.Oct 5 2022, 1:59 PM
flang/runtime/complex-powi.cpp
87

Another proposal: we could define float_Complex_t and double_Complex_t to be struct {float; float;} and struct {doule; double} instead of _Fcomplex and _Dcomplex ourselves in a shared header between this and complex-reduction.h, as those are just the definitions in MSVC.

vzakhari added inline comments.Oct 5 2022, 1:59 PM
flang/runtime/complex-powi.cpp
87

The compiler doesn't directly support a complex or _Complex keyword, therefore the Microsoft implementation uses structure types to represent complex numbers.

Thanks! I must be reading every other line :)

I agree I'd rather use the stuff from complex-reduction.h and I tried that initially, but you cannot get _[FDL]complex in a C++ file without MSVC erroring out... This is the best way I found to work around those issues.

Not sure what you mean by "the best way" :)
My point is that complex-reduction.c and complex-reduction.h already solve this exact problem, and they allow using C++ templates in sum.cpp for the actual implementation. I understand that this is a little bit more work, but we gain the uniformity, which seems important to me.

DavidTruby added inline comments.Oct 5 2022, 2:04 PM
flang/runtime/complex-powi.cpp
87

My point is that complex-reduction.c and complex-reduction.h already solve this exact problem, and they allow using C++ templates in sum.cpp for the actual implementation. I understand that this is a little bit more work, but we gain the uniformity, which seems important to me.

Sorry let me clarify; you can't include complex-reduction.h in a C++ file portably because you can't transitively include complex.h or ccomplex in MSVC without an error.
I tried to take the definitions of float_Complex_t and double_Complex_t from complex-reduction.h into a shared header, and this is the first issue I hit when trying to include it here.

Even if we rewrite this file itself in C and use macros for generating functions of all the correct types (which I think would be a lot more messy!) we still can't include the abstraction used in complex-reduction.h in the gtest file, because gtest only supports C++. So at some point we have to separately make the definition that Dcomplex and Fcomplex are {double; double} and {float; float} on Windows, whether that be in the implementation or the tests. Given this fact I'd rather keep the cleaner implementation without macros.

vzakhari added inline comments.Oct 5 2022, 2:16 PM
flang/runtime/complex-powi.cpp
87

Sorry, I forgot about the gtest. I suppose we can create wrappers in an extra C file inside flang/unittests/Runtime/: the wrappers will have struct {real; real;} arguments rather than complex and will also return struct {real; real;}, and the wrappers will call the runtime functions using proper C complex types by repacking their arguments and the return values from the runtime. Then in flang/unittests/Runtime/Complex.cpp we will just declare those wrappers as extern "C" functions without using any complex data types.

DavidTruby added inline comments.Oct 5 2022, 2:22 PM
flang/runtime/complex-powi.cpp
87

That could work but I don't feel that it buys us anything over just defining Dcomplex and Fcomplex ourselves and having a much neater implementation for the powi.

Since we know from the MSVC docs that Dcomplex is just {double; double} and Fcomplex is just {float; float}, I don't feel that there's any benefit using the MSVC definitions from the "poisonous" header than just writing our own definitions that are identical in both layout and ABI.

Essentially I'm reticent to make the general code for powi less readable, writeable and testable just to work around MSVC. I think we should do MSVC workarounds _for MSVC_ and not force them on every compiler.

DavidTruby added inline comments.Oct 5 2022, 2:24 PM
flang/runtime/complex-powi.cpp
87

Additionally, FWIW, because _Fcomplex and _Dcomplex really are just structs, they don't have any operators defined on them. So it's not even as simple as wrapping the tgpowi I've got here in a macro in a C file as that still won't work in MSVC. It would require entirely separate definitions for MSVC and non-MSVC as z * z is only available as a function in C in MSVC, not as an operator.

klausler added inline comments.Oct 5 2022, 2:27 PM
flang/runtime/complex-powi.cpp
87

Complex multiplication can just be written in terms of components, as can the division into (1.,0) for the reciprocal.

DavidTruby added inline comments.Oct 5 2022, 2:32 PM
flang/runtime/complex-powi.cpp
87

It can indeed. However, accessing the components is done differently in MSVC as for C complex numbers, so it'd still need a separate implementation (or macros every time you access re and im), and doing it this way would still make the code less clear and readable than it is right now.

My strong preference is for MSVC workarounds to be limited to MSVC branches and not litter the generic code.

Thank you, David. Let's keep it as it is right now.

Fix MSVC wrapper bug

vzakhari accepted this revision.Oct 10 2022, 1:09 PM

Thanks!

This revision is now accepted and ready to land.Oct 10 2022, 1:09 PM
This revision was landed with ongoing or failed builds.Oct 11 2022, 5:35 AM
This revision was automatically updated to reflect the committed changes.