Even more optimal Ord implementation for integers #64082
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ rollup |
|
📌 Commit b5e8f35ced2bd7fe8c3eb5029e0139f0c78316ce has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- Could we just call signum to implement this? |
bors
added
S-waiting-on-author
|
@bors r+ eh, whatever |
|
📌 Commit b5e8f35ced2bd7fe8c3eb5029e0139f0c78316ce has been approved by |
bors
added
S-waiting-on-bors
|
This seems to make some code worse: godbolt. |
|
A fair point, although I feel like comparing @bors r- to give it some time for comments. |
bors
added
S-waiting-on-author
I assumed
Hmm, that's an interesting failure case: https://godbolt.org/z/xwIxKw %0 = icmp ugt i32 %a.val, %b.val, !dbg !9
%1 = icmp ult i32 %a.val, %b.val, !dbg !12
%2 = zext i1 %1 to i64, !dbg !9
%3 = xor i64 %2, -1, !dbg !9
%.off = sext i1 %0 to i64, !dbg !13
%switch = icmp eq i64 %3, %.off, !dbg !13Looks like the bitwise-not ( |
|
I have also found that replacing the match with instead of having the unreachable_unchecked will improve the codegen slightly, which is entirely an LLVM issue. |
Can you elaborate, @nagisa? I seem to be missing something: https://godbolt.org/z/P0rVcN |
|
It helps in the particular case where the return value is compared to specific value that was posted above. https://godbolt.org/z/WD-xY4 |
|
Hmm, apparently the |
|
Kicking the PR: |
|
Also consider checking other architectures or perhaps conditionally gate this change on x86_64 |
|
I'm also curious how the change performs once inlined, like in some sorting algorithm or similar. The compiler will probably change the generated code a lot once it knows how the return value is used. E.g., if the caller is doing |
|
I agree with parched, probably a more real benchmark is in order, like sorting an array of integers. |
|
Looks like I can't coerce LLVM into optimizing this correctly, so I'll file a bug there and abandon this. |
Micro-optimize Ord::cmp for primitives I originally started looking into this because in MIR, `PartialOrd::cmp` is _huge_ and even for trivial types like `u32` which are theoretically a single statement to compare, the `PartialOrd::cmp` impl doesn't inline. A significant contributor to the size of the implementation is that it has two comparisons. And this actually follows through to the final x86_64 codegen too, which is... strange. We don't need two `cmp` instructions in order to do a single Rust-level comparison. So I started tweaking the implementation, and came up with the same thing as rust-lang#64082 (which I didn't know about at the time), I ran `llvm-mca` on it per the issue which was linked in the code to establish that it looked better, and submitted it for a benchmark run. The initial benchmark run regresses basically everything. By looking through the cachegrind diffs in the perf report then the `perf annotate` for regressed functions, I was able to identify one source of the regression: `Ord::min` and `Ord::max` no longer optimize well. Tweaking them to bypass `Ord::cmp` removed some regressions, but not much. Diving back into the cachegrind diffs and disassembly, I found one huge widespread issue was that the codegen for `Span`'s `hash_stable` regressed because `span_data_to_lines_and_cols` no longer inlined into it, because that function does a lot of `Range<BytePos>::contains`. The implementation of `Range::contains` uses `PartialOrd` multiple times, and we had massively regressed the codegen of `Range::contains`. The root problem here seems to be that `PartialOrd` is derived on `BytePos`, which is a simple wrapper around a `u32`. So for `BytePos`, `PartialOrd::{le, lt, ge, gt}` use the default impls, which go through `PartialOrd::cmp`, and LLVM fails to optimize these combinations of methods with the new `Ord::cmp` implementation. At a guess, the new implementation makes LLVM totally loses track of the fact that `<Ord for u32>::cmp` is an elaborate way to compare two integers. So I have low hopes for this overall, because my strategy (which is working) to recover the regressions is to avoid the "faster" implementation that this PR is based around. If we have to settle for an implementation of `Ord::cmp` which is on its own sub-optimal but is optimized better in combination with functions that use its return value in specific ways, so be it. However, one of the runs had an improvement in `coercions`. I don't know if that is jitter or relevant. But I'm still finding threads to pull here, so I'm going to keep at it. For the moment I am hacking up the implementations on `BytePos` instead of modifying the code that `derive(PartialOrd, Ord)` expands to because that would be hard, and it would also mean that we would just expand to more code, perhaps regressing compile time for that reason, even if the generated assembly is more efficient. --- Hacking up the remainder of the `PartialOrd`/`Ord` methods on `BytePos` took us down to 3 regressions and 6 improvements, which is interesting. All the improvements are in `coercions`, so I'm sure this improved _something_ but whether it matters... hard to say. Based on the findings of `@joboet,` I'm going to cherry-pick rust-lang#106065 onto this branch, because that strategy seems to improve `PartialOrd::lt` and `PartialOrd::ge` back to the original codegen, even when they are using our new `Ord::cmp` impl. If the remaining perf regressions are due to de-optimizing a `PartialOrd::lt` not on `BytePos`, this might be a further improvement. --- Okay, that cherry-pick brought us down to 2 regressions but that might be noise. We still have the same 6 improvements, all on `coercions`. I think the next thing to try here is modifying the implementation of `derive(PartialOrd)` to automatically emit the modifications that I made to `BytePos` (directly implementing all the methods for newtypes). But even if that works, I think the effect of this change is so mixed that it's probably not worth merging with current LLVM. What I'm afraid of is that this change currently pessimizes matching on `Ordering`, and that is the most natural thing to do with an enum. So I'm not closing this yet, but I think without a change from LLVM, I have other priorities at the moment. r? `@ghost`
When I saw #63767 it made me think of #61635, and I noticed I could use the same trick from the latter of those for
cmp: https://godbolt.org/z/XCDDt7Before:
After:
Old llvm-mca statistics:
New llvm-mca statistics:
r? @nagisa