Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Daniel Krügler-3
Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland
<[hidden email]>:

>
> Greetings,
>
> This is late but I wanted to put it out there just to finish a thing.
>
> It's fairly straightforward constexpr of operators and some simple
> functions for std::complex.
>
> The only thing that jumped out was the norm function.  We had this:
>
>      struct _Norm_helper<true>
>      {
>        template<typename _Tp>
>          static inline _Tp _S_do_it(const complex<_Tp>& __z)
>          {
>            _Tp __res = std::abs(__z);
>            return __res * __res;
>          }
>      };
>
> Since abs can't be made constexpr for complex since it involves sqrt (It
> probably could but that's another story) I had to fall back to the x^2 +
> y^2.  I don't know who that will bother.  This version should be faster
> and I can't think of any useful trustworthy difference numerically
> either in terms of accuracy of stability.
>
> Barring any feedback on that I'll clean it up and maybe rename my tests
> from constexpr_all_the_things.cc to more_constexpr.cc ;-)
>
> It builds and tests cleanly on x86_64-linux.

Hmmh, according to the recent working draft the following complex
functions are *not* constexpr:

arg, proj

So, shouldn't their new C++20-constexpr specifiers be added
conditionally (In the sense of gcc extensions)?

- Daniel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Ed Smith-Rowland
On 11/16/18 12:38 PM, Daniel Krügler wrote:

> Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland
> <[hidden email]>:
>> Greetings,
>>
>> This is late but I wanted to put it out there just to finish a thing.
>>
>> It's fairly straightforward constexpr of operators and some simple
>> functions for std::complex.
>>
>> The only thing that jumped out was the norm function.  We had this:
>>
>>       struct _Norm_helper<true>
>>       {
>>         template<typename _Tp>
>>           static inline _Tp _S_do_it(const complex<_Tp>& __z)
>>           {
>>             _Tp __res = std::abs(__z);
>>             return __res * __res;
>>           }
>>       };
>>
>> Since abs can't be made constexpr for complex since it involves sqrt (It
>> probably could but that's another story) I had to fall back to the x^2 +
>> y^2.  I don't know who that will bother.  This version should be faster
>> and I can't think of any useful trustworthy difference numerically
>> either in terms of accuracy of stability.
>>
>> Barring any feedback on that I'll clean it up and maybe rename my tests
>> from constexpr_all_the_things.cc to more_constexpr.cc ;-)
>>
>> It builds and tests cleanly on x86_64-linux.
> Hmmh, according to the recent working draft the following complex
> functions are *not* constexpr:
>
> arg, proj
>
> So, shouldn't their new C++20-constexpr specifiers be added
> conditionally (In the sense of gcc extensions)?
>
> - Daniel
>
I did not see that those were not constexpr.  I guess arg needs atan2
and proj

I'll remove the constexpr on those and adjust everything.

Thank you.

Ed


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Ed Smith-Rowland
On 11/16/18 3:53 PM, Ed Smith-Rowland wrote:

> On 11/16/18 12:38 PM, Daniel Krügler wrote:
>> Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland
>> <[hidden email]>:
>>> Greetings,
>>>
>>> This is late but I wanted to put it out there just to finish a thing.
>>>
>>> It's fairly straightforward constexpr of operators and some simple
>>> functions for std::complex.
>>>
>>> The only thing that jumped out was the norm function.  We had this:
>>>
>>>       struct _Norm_helper<true>
>>>       {
>>>         template<typename _Tp>
>>>           static inline _Tp _S_do_it(const complex<_Tp>& __z)
>>>           {
>>>             _Tp __res = std::abs(__z);
>>>             return __res * __res;
>>>           }
>>>       };
>>>
>>> Since abs can't be made constexpr for complex since it involves sqrt
>>> (It
>>> probably could but that's another story) I had to fall back to the
>>> x^2 +
>>> y^2.  I don't know who that will bother.  This version should be faster
>>> and I can't think of any useful trustworthy difference numerically
>>> either in terms of accuracy of stability.
>>>
>>> Barring any feedback on that I'll clean it up and maybe rename my tests
>>> from constexpr_all_the_things.cc to more_constexpr.cc ;-)
>>>
>>> It builds and tests cleanly on x86_64-linux.
>> Hmmh, according to the recent working draft the following complex
>> functions are *not* constexpr:
>>
>> arg, proj
>>
>> So, shouldn't their new C++20-constexpr specifiers be added
>> conditionally (In the sense of gcc extensions)?
>>
>> - Daniel
>>
> I did not see that those were not constexpr.  I guess arg needs atan2
> and proj
>
> I'll remove the constexpr on those and adjust everything.
>
> Thank you.
>
> Ed
>
All,

Here is the cleaned up, conformant patch.

Builds and tests cleanly on x86_64-linux.

Is this OK or should I really wait?

Ed



CL_p0415 (1K) Download Attachment
patch_p0415 (47K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Jonathan Wakely-3
On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote:

>@@ -322,67 +323,43 @@
>   //@{
>   ///  Return new complex value @a x plus @a y.
>   template<typename _Tp>
>-    inline complex<_Tp>
>+    inline _GLIBCXX20_CONSTEXPR complex<_Tp>
>     operator+(const complex<_Tp>& __x, const complex<_Tp>& __y)
>-    {
>-      complex<_Tp> __r = __x;
>-      __r += __y;
>-      return __r;
>-    }
>+    { return complex<_Tp>(__x.real() + __y.real(), __x.imag() + __y.imag()); }

Is this change (and all the similar ones) really needed?

Doesn't the fact that all the constructors and member operators of
std::complex mean that the original definition is also valid in a
constexpr function?

>@@ -1163,50 +1143,43 @@
> #endif
>
>       template<typename _Tp>
>-        complex&
>+        _GLIBCXX20_CONSTEXPR complex&
>         operator=(const complex<_Tp>&  __z)
> {
>-  __real__ _M_value = __z.real();
>-  __imag__ _M_value = __z.imag();
>+  _M_value = __z.__rep();

These changes look OK, but I wonder if we shouldn't ask the compiler
to make it possible to use __real__ and __imag__ in constexpr
functions instead.

I assume it doesn't, and that's why you made this change. But if it
Just Worked, and the other changes I commented on above are also
unnecessary, then this patch would *mostly* just be adding
_GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any
dialects except C++2a).


>@@ -1872,7 +1831,7 @@
>     { return _Tp(); }
>
>   template<typename _Tp>
>-    inline typename __gnu_cxx::__promote<_Tp>::__type
>+    _GLIBCXX_CONSTEXPR inline typename __gnu_cxx::__promote<_Tp>::__type

This should be _GLIBCXX20_CONSTEXPR.

>Index: testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>===================================================================
>--- testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc (nonexistent)
>+++ testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc (working copy)
>@@ -0,0 +1,51 @@
>+// { dg-do compile { target c++2a } }

All the tests with { target c++2a} should also have:

// { dg-options "-std=gnu++2a" }

Because otherwise they are skipped by default, and only get run when
RUNTESTFLAGS explicitly includes something like
--target_board=unix/-std=gnu++2a

The dg-options needs to come first, or it doesn't apply before the
check for { target c++2a }.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Ed Smith-Rowland
On 11/19/18 6:13 AM, Jonathan Wakely wrote:

> On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote:
>> @@ -322,67 +323,43 @@
>>   //@{
>>   ///  Return new complex value @a x plus @a y.
>>   template<typename _Tp>
>> -    inline complex<_Tp>
>> +    inline _GLIBCXX20_CONSTEXPR complex<_Tp>
>>     operator+(const complex<_Tp>& __x, const complex<_Tp>& __y)
>> -    {
>> -      complex<_Tp> __r = __x;
>> -      __r += __y;
>> -      return __r;
>> -    }
>> +    { return complex<_Tp>(__x.real() + __y.real(), __x.imag() +
>> __y.imag()); }
>
> Is this change (and all the similar ones) really needed?
>
> Doesn't the fact that all the constructors and member operators of
> std::complex mean that the original definition is also valid in a
> constexpr function?
These changes are rolled back. Sorry.

>> @@ -1163,50 +1143,43 @@
>> #endif
>>
>>       template<typename _Tp>
>> -        complex&
>> +        _GLIBCXX20_CONSTEXPR complex&
>>         operator=(const complex<_Tp>&  __z)
>>     {
>> -      __real__ _M_value = __z.real();
>> -      __imag__ _M_value = __z.imag();
>> +      _M_value = __z.__rep();
>
> These changes look OK, but I wonder if we shouldn't ask the compiler
> to make it possible to use __real__ and __imag__ in constexpr
> functions instead.
>
> I assume it doesn't, and that's why you made this change. But if it
> Just Worked, and the other changes I commented on above are also
> unnecessary, then this patch would *mostly* just be adding
> _GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any
> dialects except C++2a).
Yes, this is the issue.  I agree that constexpr _real__, __imag__would
be better.

Do you have any idea where this change would be?  I grepped around a
little and couldn't figure it out.  if you don't I'll look more.

Actually, looking at constexpr.c it looks like the old way ought to work...

OK, plain assignment works but not the others.  Interesting.

>
>> @@ -1872,7 +1831,7 @@
>>     { return _Tp(); }
>>
>>   template<typename _Tp>
>> -    inline typename __gnu_cxx::__promote<_Tp>::__type
>> +    _GLIBCXX_CONSTEXPR inline typename
>> __gnu_cxx::__promote<_Tp>::__type
>
> This should be _GLIBCXX20_CONSTEXPR.
Done.

>> Index:
>> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> ===================================================================
>> ---
>> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> (nonexistent)
>> +++
>> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> (working copy)
>> @@ -0,0 +1,51 @@
>> +// { dg-do compile { target c++2a } }
>
> All the tests with { target c++2a} should also have:
>
> // { dg-options "-std=gnu++2a" }
>
> Because otherwise they are skipped by default, and only get run when
> RUNTESTFLAGS explicitly includes something like
> --target_board=unix/-std=gnu++2a
>
> The dg-options needs to come first, or it doesn't apply before the
> check for { target c++2a }.
>
Thank you, done.

Updated patch attached.  I'd like to understand why

     __real__ _M_value += __z.real();

doesn't work though.

Ed



patch_p0415_2 (46K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, libstdc++] Implement P0415 More constexpr for std::complex.

Jonathan Wakely-3
On 26/11/18 09:30 +0100, Christophe Lyon wrote:

>On Thu, 22 Nov 2018 at 10:20, Jonathan Wakely <[hidden email]> wrote:
>>
>> On 20/11/18 17:58 -0500, Ed Smith-Rowland wrote:
>> >On 11/19/18 6:13 AM, Jonathan Wakely wrote:
>> >>On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote:
>> >>>@@ -322,67 +323,43 @@
>> >>>  //@{
>> >>>  ///  Return new complex value @a x plus @a y.
>> >>>  template<typename _Tp>
>> >>>-    inline complex<_Tp>
>> >>>+    inline _GLIBCXX20_CONSTEXPR complex<_Tp>
>> >>>    operator+(const complex<_Tp>& __x, const complex<_Tp>& __y)
>> >>>-    {
>> >>>-      complex<_Tp> __r = __x;
>> >>>-      __r += __y;
>> >>>-      return __r;
>> >>>-    }
>> >>>+    { return complex<_Tp>(__x.real() + __y.real(), __x.imag() +
>> >>>__y.imag()); }
>> >>
>> >>Is this change (and all the similar ones) really needed?
>> >>
>> >>Doesn't the fact that all the constructors and member operators of
>> >>std::complex mean that the original definition is also valid in a
>> >>constexpr function?
>> >These changes are rolled back. Sorry.
>> >>>@@ -1163,50 +1143,43 @@
>> >>>#endif
>> >>>
>> >>>      template<typename _Tp>
>> >>>-        complex&
>> >>>+        _GLIBCXX20_CONSTEXPR complex&
>> >>>        operator=(const complex<_Tp>&  __z)
>> >>>    {
>> >>>-      __real__ _M_value = __z.real();
>> >>>-      __imag__ _M_value = __z.imag();
>> >>>+      _M_value = __z.__rep();
>> >>
>> >>These changes look OK, but I wonder if we shouldn't ask the compiler
>> >>to make it possible to use __real__ and __imag__ in constexpr
>> >>functions instead.
>> >>
>> >>I assume it doesn't, and that's why you made this change. But if it
>> >>Just Worked, and the other changes I commented on above are also
>> >>unnecessary, then this patch would *mostly* just be adding
>> >>_GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any
>> >>dialects except C++2a).
>> >
>> >Yes, this is the issue.  I agree that constexpr _real__, __imag__would
>> >be better.
>> >
>> >Do you have any idea where this change would be?  I grepped around a
>> >little and couldn't figure it out.  if you don't I'll look more.
>>
>> No idea, sorry.
>>
>> >Actually, looking at constexpr.c it looks like the old way ought to work...
>> >
>> >OK, plain assignment works but not the others.  Interesting.
>> >
>> >>
>> >>>@@ -1872,7 +1831,7 @@
>> >>>    { return _Tp(); }
>> >>>
>> >>>  template<typename _Tp>
>> >>>-    inline typename __gnu_cxx::__promote<_Tp>::__type
>> >>>+    _GLIBCXX_CONSTEXPR inline typename
>> >>>__gnu_cxx::__promote<_Tp>::__type
>> >>
>> >>This should be _GLIBCXX20_CONSTEXPR.
>> >Done.
>> >>>Index:
>> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> >>>===================================================================
>> >>>---
>> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> >>>(nonexistent)
>> >>>+++
>> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc
>> >>>(working copy)
>> >>>@@ -0,0 +1,51 @@
>> >>>+// { dg-do compile { target c++2a } }
>> >>
>> >>All the tests with { target c++2a} should also have:
>> >>
>> >>// { dg-options "-std=gnu++2a" }
>> >>
>> >>Because otherwise they are skipped by default, and only get run when
>> >>RUNTESTFLAGS explicitly includes something like
>> >>--target_board=unix/-std=gnu++2a
>> >>
>> >>The dg-options needs to come first, or it doesn't apply before the
>> >>check for { target c++2a }.
>> >>
>> >Thank you, done.
>>
>> OK for trunk, thanks.
>>
>> >Updated patch attached.  I'd like to understand why
>> >
>> >    __real__ _M_value += __z.real();
>> >
>> >doesn't work though.
>>
>> Yes, I agree it should. If you don't figure it out please file a bug
>> requesting that it works, so somebody else might look into it.
>>
>
>Hi,
>I have noticed that
>FAIL: 26_numerics/complex/requirements/more_constexpr.cc
>on arm and aarch64
>The error messages:
>Excess errors:
>/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168:
>error: '__float128' was not declared in this scope
>/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168:
>error: no matching function for call to
>'test_operator_members<<expression error>, __float128>()'
>/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168:
>error: template argument 1 is invalid
Should be fixed by this patch, committed to trunk.



patch.txt (1016 bytes) Download Attachment