On Thu, 28 Aug 2025 at 08:29, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Wed, Aug 27, 2025 at 6:58 PM Jakub Jelinek <ja...@redhat.com> wrote:
>>
>> On Wed, Aug 27, 2025 at 03:18:55PM +0200, Tomasz Kamiński wrote:
>> > For any minimum value of a signed type, its negation (with wraparound) 
>> > results
>> > in the same value, behaving like zero. Representing the unordered result 
>> > with
>> > this minimum value, along with 0 for equal, 1 for greater, and -1 for less
>> > in partial_ordering, allows its value to be reversed using unary negation.
>> >
>> > The operator<=(partial_order, 0) now checks if the reversed value is 
>> > positive.
>> > This works correctly because the unordered value remains unchanged and thus
>> > negative.
>> >
>> > libstdc++-v3/ChangeLog:
>> >
>> >       * libsupc++/compare (_Ncmp::_Unordered): Rename and change the value
>> >       to minimum value of signed char.
>> >       (_Ncomp::unordered): Renamed from _Unordered, the name is reserved
>> >       by partial_ordered::unordered.
>> >       (partial_ordering::_M_reverse()): Define.
>> >       (operator<=(partial_ordering, __cmp_cat::__unspec))
>> >       (operator>=(__cmp_cat::__unspec, partial_ordering)): Implemented
>> >       in terms of negated _M_value.
>> >       (operator>=(partial_ordering, __cmp_cat::__unspec))
>> >       (operator<=(__cmp_cat::__unspec, partial_ordering)): Directly
>> >       compare _M_value, as unordered value is negative.
>> >       (partial_ordering::unordered): Handle _Ncmp::unoredred rename.
>> >       * python/libstdcxx/v6/printers.py: Add -128 as integer value
>> >       for unordered, keeping 2 to preserve backward compatibility.
>> >
>> > Reviewed-by: Jonathan Wakely <jwak...@redhat.com>
>> > Signed-off-by: Tomasz Kamiński <tkami...@redhat.com>
>>
>> This broke
>> +FAIL: g++.dg/opt/pr94589-2.C  -std=gnu++20  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|==|!=|>|>=) 5\\\\.0" 12
>> +FAIL: g++.dg/opt/pr94589-2.C  -std=gnu++23  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|==|!=|>|>=) 5\\\\.0" 12
>> +FAIL: g++.dg/opt/pr94589-2.C  -std=gnu++26  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|==|!=|>|>=) 5\\\\.0" 12
>> on i686-linux, x86_64-linux or powerpc64le-linux, additionally
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++20  scan-tree-dump-times optimized 
>> "[ij]_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) [ij]_[0-9]+\\\\(D\\\\)" 8
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++20  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) 5\\\\.0" 8
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++23  scan-tree-dump-times optimized 
>> "[ij]_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) [ij]_[0-9]+\\\\(D\\\\)" 8
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++23  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) 5\\\\.0" 8
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++26  scan-tree-dump-times optimized 
>> "[ij]_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) [ij]_[0-9]+\\\\(D\\\\)" 8
>> FAIL: g++.dg/opt/pr94589-5.C  -std=gnu++26  scan-tree-dump-times optimized 
>> "i_[0-9]+\\\\(D\\\\) (?:<|<=|>|>=) 5\\\\.0" 8
>> on powerpc64le-linux.
>> Both tree-ssa-phiopt.cc (spaceship_replacement)
>> and tree-ssa-math-opts.cc (optimize_spaceship)
>> attempt to optimize some spaceship comparisons.
>
> I have no experience with backend optimization, so I would need help to fix 
> the above.

This is "middle-end", i.e. the target-independent optimizations.
Back-end would be e.g. x86-specific or arm-specific optimizations.

> From my side I could only offer reverting the change, but I do not think we 
> should do so,
> see below.
>
>>
>> Although I wrote both, my memory is weak, so just judging from function
>> comments, the latter likely should deal with values -1, 0, 1 and X where
>> X is some other value, so perhaps all we want to change there is
>>       other than -1, 0, 1, for libstdc++ 2, for libc++ -127.  */
>> to
>>       other than -1, 0, 1, for libstdc++ -128, for libc++ -127.  */
>> and adjust other comments that show the 2 value to show -128 instead.
>> But tree-ssa-phiopt.cc clearly optimizes only the 2 case and has pattern
>> recognition of the (x & ~1) == 0 and similar cases, which all will now be
>> different for -128.  So, either spaceship_replacement needs to be rewritten
>> to only handle -128 instead of 2, or depending on the source either -128 or
>> 2.
>
> I have selected the value -128, because it should not require any special 
> optimization
> for spaceship replacement. The (x & ~1) == 0 are most likely product of 
> implementation
> of one of the following functions:
>      operator>=(partial_ordering __v, __cmp_cat::__unspec) noexcept
>     { return __cmp_cat::type(__v._M_value & 1) == __v._M_value;
>
>      operator<=>(__cmp_cat::__unspec, partial_ordering __v) noexcept
>      {
>        if (__v._M_value & 1)
>         return partial_ordering(__cmp_cat::_Ord(-__v._M_value));
>       else
>         return __v;
>     }
>
> However, with my patch I have replaced them with:
>      operator>=(partial_ordering __v, __cmp_cat::__unspec) noexcept
>      { return __v._M_value >= 0; } // unordered is now negative, so positive 
> check is simple
>
>      operator<=(partial_ordering __v, __cmp_cat::__unspec) noexcept
>     { return __v._M_reverse() >= 0;  } // negating -128 in 8bit produces 
> -128, so unordered remains negative
>                                                           // so simple 
> positive check is fine
>
>      operator<=>(__cmp_cat::__unspec, partial_ordering __v) noexcept
>      { return partial_ordering(__cmp_cat::_Ord(__v._M_reverse())); }
> Where _M_reverse is simply doing static_cast<__cmp_cat::type>(-_M_value).
> That means that we are now using just simple negation of 8 bit value (the cast
> is truncating it back from integer).
>
> I have updated the reversing operator<=> here r16-3417-gb3038e1ace2a4e,
> and now is doing the same operations as for strong and weak order.

Jakub and I looked at the generated assembly for this last night, and
it's different but not really worse for two of the operators, and it's
much better (and branch-free) for 0 <=> partorder.

I'm not sure the right way to fix the tests (maybe just remove those
checks, if we don't need to handle the (v & ~1) >= 0 pattern at all
now?) but I'll wait for Jakub to suggest what to do.

Reply via email to