Skip to content

std: Rework std.math.order and std.math.Order#23387

Open
SeanTUT wants to merge 20 commits intoziglang:masterfrom
SeanTUT:order-patch
Open

std: Rework std.math.order and std.math.Order#23387
SeanTUT wants to merge 20 commits intoziglang:masterfrom
SeanTUT:order-patch

Conversation

@SeanTUT
Copy link
Copy Markdown
Contributor

@SeanTUT SeanTUT commented Mar 27, 2025

Revival of #23291

  • std.math.{order, Order} are now implemented in their own file (std/math/order.zig)
  • The values of Order are now explicitly specified
    • To be specific, the values are as follows:
      • lt = -1
      • eq = 0
      • gt = 1
    • Leveraging this enabled some optimizations for Order.invert and Order.compare
  • std.math.order is now implemented branchlessly, for both floats and integers
  • std.math.order for floats now uses IEEE total ordering
    • That is to say, numbers are ordered as follows: {-nan, -inf, -real, -0, +0, +real, inf, nan}
    • Previously, ordering nans would result in illegal behavior. Now, this is well defined.
    • This is not a breaking change, as it only affects cases which were previously illegal
      • This does have some implications for std.math.order users: 0.0 and -0.0 and no longer treated as equal, in contrast to regular IEEE comparisons, where 0.0 == -0.0
  • Additionally fixed std.math.big.int.subCarry returning -0 when subtracting 0 - 0. This had in turn caused some compiler bugs with comptime arithmetic, which made themselves apparent while working on this
    • This bug was why I closed the original PR

SeanTUT and others added 11 commits March 18, 2025 12:19
- Moved implementations of `std.math.order` and `std.math.Order` into
  their own file (lib/std/math/order.zig)
- Floats use total ordering, eliminating the possibility of reaching unreachable code in `std.math.order`
- `.lt`, `.eq`, and `.gt` now have vales of `-1`, `0`, and `1`
  respectively, allowing for some optimizations in ordering integers inverting orderings
- `std.mem.order` now utilizes `std.math.Order.differ` instead of
  manually implementing the functionality
- Modified up some existing tests to use `std.testing.expectEqual`
  instead of `std.testing.expect` when appropriate
In math/order.zig, cast the results of the comparison to i8 before
subtracting. This sidesteps the discovered bug.
This test was intended for investigating a compiler bug which will be
looked into seperately
Previously this would result in -0, which resulted in compiler bugs with
comptime arithmetic (affecting switch, @enumFromInt, and more).

With this bug fixed, several improvements in std.math.order became
possible, including branchless std.math.order and optimized
std.math.Order.{invert, compare}
Integer literals are now used again instead of runtime integer constants
Fixes comptime comparison of `0.0` and `-0.0`
`
Comment thread lib/std/math/order.zig
@SeanTUT
Copy link
Copy Markdown
Contributor Author

SeanTUT commented Mar 28, 2025

As is indicated by the repeated failure of tests, this change is becoming much more involved than it seemed at a glance. I expect to have this finished in a few days.

@SeanTUT
Copy link
Copy Markdown
Contributor Author

SeanTUT commented Mar 31, 2025

A follow-up change might be to edit std.sort.{asc, desc} to use the new order, but I will be leaving that be for now

SeanTUT and others added 4 commits March 31, 2025 12:50
The possibolity of negation flipping the signal nan bit is now taken
into account
Forgoes the branchless implementation in stage2_riscv64 entirely to
enable support
@SeanTUT
Copy link
Copy Markdown
Contributor Author

SeanTUT commented Apr 3, 2025

Now that this is (finally, haha) done and in working order, I would like to hear any opinions on the breakage made by total ordering. This only changes how zeros of different signs are ordered, so it is a niche case, but it could be worth considering. Despite that, I think total ordering is the sanest way to go about this; it is a part of the IEEE 754 standard and covers all edge cases.

If we want to eliminate breakage, we could split the current order into order and totalOrder, but that would beg the question of how order would handle nan values, as frankly, illegal behavior doesn't seem like a tenable option here, at least in my opinion (we don't want to cause panics anytime someone tries to do a simple floating point comparison!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants