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

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

Merged

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Dec 12, 2021

Draft because it needs a rebase after #91766 eventually gets through bors.

This enables the optimizations from #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

pub fn demo(x: &[u8], y: [u8; 4]) -> bool {
    *x == y
}

Currently writes the array to stack for no reason:

	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:

	cmp	rsi, 4
	jne	.LBB1_1
	cmp	dword ptr [rdi], edx
	sete	al
	ret

.LBB1_1:
	xor	eax, eax
	ret

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 12, 2021
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the array-slice-eq-via-arrays-not-slices branch from 41664e8 to 363987b Compare December 12, 2021 21:19
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the array-slice-eq-via-arrays-not-slices branch from 363987b to 97553c0 Compare December 12, 2021 22:59
@scottmcm
Copy link
Member Author

I had to update a codegen test, so preemptively
@bors rollup=iffy

@scottmcm scottmcm added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 12, 2021
@Urgau
Copy link
Member

Urgau commented Dec 13, 2021

Cool optimization, but all of these "hacks" are starting to worrying me. They all feels too targeted. It seems to me that they could benefit to way much code rather than just the one that are in core. It's seems to me that the compiler is missing some optimization for the general case.

This'll still go via slices eventually for large arrays, but this way slice comparisons to short arrays can use the same memcmp-avoidance tricks.

Added some tests for all the combinations to make sure I didn't accidentally infinitely-recurse something.
@scottmcm scottmcm force-pushed the array-slice-eq-via-arrays-not-slices branch from 97553c0 to a0b9690 Compare December 14, 2021 21:20
@scottmcm scottmcm marked this pull request as ready for review December 14, 2021 21:20
@scottmcm scottmcm removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 14, 2021
@scottmcm
Copy link
Member Author

@Urgau I went and filed an LLVM issue (llvm/llvm-project#52701) for the core thing that #85828 was made to avoid.

I think this PR is actually reasonable even if the LLVM issue is fixed, though -- there's no reason to lose the "hey, this is only ever fixed-length" part of slice-array comparisons by always dropping to slice-slice immediately. For example, MIR optimizations would have a much easier time folding things if it doesn't need to inspect slice lengths to know what's happening. And I think *self == **other is in general the right way to implement eq for mixed-levels-of-references, rather than doing the array-specific self[..] == other[..] here.

The whole IsRawEqComparable dance -- which isn't actually touched in this PR -- could potentially be removed in future in favour of things like the safe transmute work. It's basically a combination of HasNoPadding (aka CanBeSafetyTransmutedToBytes) and a deep version of StructuralEq (see

/// Primitive types (`u32`, `str`) have structural equality by definition. For composite data
/// types, equality for the type as a whole is structural when it is the same as equality
/// between all components (fields, array elements, etc.) of that type. For ADTs, structural
/// equality is indicated by an implementation of `PartialStructuralEq` and `StructuralEq` for
/// that type.
///
/// This function is "shallow" because it may return `true` for a composite type whose fields
/// are not `StructuralEq`. For example, `[T; 4]` has structural equality regardless of `T`
/// because equality for arrays is determined by the equality of each array element. If you
/// want to know whether a given call to `PartialEq::eq` will proceed structurally all the way
/// down, you will need to use a type visitor.
#[inline]
pub fn is_structural_eq_shallow(&'tcx self, tcx: TyCtxt<'tcx>) -> bool {
).

@dtolnay
Copy link
Member

dtolnay commented Dec 14, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 14, 2021

📌 Commit a0b9690 has been approved by dtolnay

@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 14, 2021
@bors
Copy link
Contributor

bors commented Dec 17, 2021

⌛ Testing commit a0b9690 with merge 7abab1e...

@bors
Copy link
Contributor

bors commented Dec 17, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 7abab1e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2021
@bors bors merged commit 7abab1e into rust-lang:master Dec 17, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 17, 2021
@scottmcm scottmcm deleted the array-slice-eq-via-arrays-not-slices branch December 17, 2021 22:17
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7abab1e): comparison url.

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

  • Small improvement in instruction counts (up to -1.7% on full builds of piston-image)
  • Moderate regression in instruction counts (up to 1.7% on incr-patched: b9b3e592dd cherry picked builds of style-servo)

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

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