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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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...
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 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.
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.
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.
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<>. |
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
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 :) |
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
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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 | ||
33 | typo here and below: float -> double This causes the test failure on Windows. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | It's here in the docs:
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 | ||
33 | I assumed it would be a daft mistake. Thanks! |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 |
Thanks! I must be reading every other line :)
Not sure what you mean by "the best way" :) |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 |
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. 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. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | Complex multiplication can just be written in terms of components, as can the division into (1.,0) for the reciprocal. |
flang/runtime/complex-powi.cpp | ||
---|---|---|
86 | 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. |
why is RTNAME_STRINGIFY_ needed? can't this just be #x?