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.