Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow memcmp for more array comparisons #91766

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

scottmcm
Copy link
Member

This way comparing [NonZeroU8; 8] is just as fast as comparing [u8; 8].

This way comparing `[NonZeroU8; 8]` is just as fast as comparing `[u8; 8]`.
@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@yaahc
Copy link
Member

yaahc commented Dec 11, 2021

Very fancy, particularly appreciate the comments.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 11, 2021

📌 Commit 24affba has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2021
@the8472
Copy link
Member

the8472 commented Dec 11, 2021

Since it's a specialization trait it should be possible to extend this to nested arrays/slices of arrays.

@matthiaskrgr
Copy link
Member

@bors rollup=never might affect performance

@bors
Copy link
Contributor

bors commented Dec 14, 2021

⌛ Testing commit 24affba with merge 83b32f2...

@bors
Copy link
Contributor

bors commented Dec 14, 2021

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 83b32f2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2021
@bors bors merged commit 83b32f2 into rust-lang:master Dec 14, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (83b32f2): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -5.2% on incr-unchanged builds of inflate)
  • Small regression in instruction counts (up to 1.4% on incr-unchanged builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 14, 2021
@scottmcm scottmcm deleted the more-array-raw-eq branch December 15, 2021 05:02
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2021
…t-slices, r=dtolnay

Do array-slice equality via array equality, rather than always via slices

~~Draft because it needs a rebase after rust-lang#91766 eventually gets through bors.~~

This enables the optimizations from rust-lang#85828 to be used for array-to-slice comparisons too, not just array-to-array.

For example, <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=5f9ba69b3d5825a782f897c830d3a6aa>
```rust
pub fn demo(x: &[u8], y: [u8; 4]) -> bool {
    *x == y
}
```
Currently writes the array to stack for no reason:
```nasm
	sub	rsp, 4
	mov	dword ptr [rsp], edx
	cmp	rsi, 4
	jne	.LBB0_1
	mov	eax, dword ptr [rdi]
	cmp	eax, dword ptr [rsp]
	sete	al
	add	rsp, 4
	ret

.LBB0_1:
	xor	eax, eax
	add	rsp, 4
	ret
```
Whereas with the change in this PR it just compares it directly:
```nasm
	cmp	rsi, 4
	jne	.LBB1_1
	cmp	dword ptr [rdi], edx
	sete	al
	ret

.LBB1_1:
	xor	eax, eax
	ret
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants