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.
>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.
Regards,
Tomasz
>
> Jakub
>
>
