Use the chaining methods on PartialOrd for slices too #138881
+260
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
scottmcm
commented
Mar 24, 2025
|
|
||
| // CHECK-LABEL: @array_of_tuple_le | ||
| #[no_mangle] | ||
| pub fn array_of_tuple_le(a: &[(i16, u16); 2], b: &[(i16, u16); 2]) -> bool { |
There was a problem hiding this comment.
Nightly, note the setl+setg from the cmp methods:
array_of_tuple_le:
movzwl (%rdi), %eax
movzwl (%rsi), %ecx
cmpw %cx, %ax
setl %cl
setg %al
jne .LBB0_1
movzwl 2(%rdi), %eax
cmpw 2(%rsi), %ax
seta %al
sbbb $0, %al
testb %al, %al
jne .LBB0_7
jmp .LBB0_4
.LBB0_1:
subb %cl, %al
testb %al, %al
jne .LBB0_7
.LBB0_4:
movzwl 4(%rdi), %eax
movzwl 4(%rsi), %ecx
cmpw %cx, %ax
setl %cl
setg %al
jne .LBB0_5
movzwl 6(%rdi), %eax
cmpw 6(%rsi), %ax
seta %al
sbbb $0, %al
jmp .LBB0_7
.LBB0_5:
subb %cl, %al
.LBB0_7:
testb %al, %al
setle %al
retqWith this PR:
array_of_tuple_le:
movzwl (%rcx), %eax
movzwl (%rdx), %r8d
cmpw %r8w, %ax
jne .LBB1_1
movzwl 2(%rcx), %r8d
movzwl 2(%rdx), %r9d
cmpw %r9w, %r8w
jne .LBB1_2
movzwl 4(%rcx), %eax
movzwl 4(%rdx), %r8d
cmpw %r8w, %ax
jne .LBB1_1
movzwl 6(%rcx), %r8d
movzwl 6(%rdx), %r9d
movb $1, %al
cmpw %r9w, %r8w
jne .LBB1_2
retq
.LBB1_1:
cmpw %r8w, %ax
setle %al
retq
.LBB1_2:
cmpw %r9w, %r8w
setb %al
retqThat's so nice -- straight line with no jumps taken if they're equal, otherwise jumps to the check it needs (dedup'd by consistent register choices), saves that, and returns.
There was a problem hiding this comment.
Note that even with #133984 landed, we still don't optimize away the [su]cmps in nightly today: https://rust.godbolt.org/z/ss5E7q6PT.
scottmcm
commented
Mar 24, 2025
Comment on lines
+55
to
+61
| // This is certainly not the obvious way to implement these methods. | ||
| // Unfortunately, using anything that looks at the discriminant means that | ||
| // LLVM sees a check for `2` (aka `ControlFlow<bool>::Continue(())`) and | ||
| // gets very distracted by that, ending up generating extraneous code. | ||
| // This should be changed to something simpler once either LLVM is smarter, | ||
| // see <https://github.com/llvm/llvm-project/issues/132678>, or we generate | ||
| // niche discriminant checks in a way that doesn't trigger it. |
There was a problem hiding this comment.
So there's a backlink: llvm/llvm-project#132678
|
Oh, I'm glad this passes in LLVM18 too. As the one with context from #138135, |
scottmcm
removed
the
T-compiler
|
☔ The latest upstream changes (presumably #138956) made this pull request unmergeable. Please resolve the merge conflicts. |
scottmcm
force-pushed
the
more-chaining-ord
branch
from
17ab2d3 to
877f8de
Compare
This comment has been minimized.
This comment has been minimized.
scottmcm
force-pushed
the
more-chaining-ord
branch
from
877f8de to
cf991d9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#138135 added these doc-hidden trait methods to improve the tuple codegen. This PR adds more implementations and callers so that the codegen for slice (and array) comparisons also improves.