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

Optimize SbTag::eq #2218

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Optimize SbTag::eq #2218

merged 3 commits into from
Jun 8, 2022

Conversation

Noratrieb
Copy link
Member

The code before generated really bad code with a branch.
This nudges LLVM towards being smarter and simply comparing
the integers.

See #2214 (comment)

The code before generated really bad code with a branch.
This nudges LLVM towards being smarter and simply comparing
the integers.
@Noratrieb
Copy link
Member Author

This change was suggested by @saethlin somewhere, but I don't recall where.

@Noratrieb
Copy link
Member Author

That's an interesting clippy lint, first time I've seen it

@saethlin
Copy link
Member

saethlin commented Jun 8, 2022

I suggested this here: rust-lang/rust#49892 (comment)

How does this compare to #2214 ? Obviously we could have both of these (and this change plays nicely with #1935 because it also benefits the cache lookup code path) but it would be good to have benchmarks for both.

Generally I'm okay with the sort of change in this PR though I'd rather solve this codegen problem by improving derive(PartialEq) rather than using a non-obvious implementation like this. The explanation for why we have this is "rustc/LLVM is producing bad code so we're hacking around it" as opposed to something like #2214 where the explanation is "Stacked Borrows has a particular property and we're reflecting that in this implementation".

Also as a sidenote, all this optimization in this and #2214 is very much in the weeds unless you rebase those onto #1935 which I promise I will make progress on soon.

@saethlin
Copy link
Member

saethlin commented Jun 8, 2022

Also since we're dealing with such small optimizations I wonder if this intersects with #2132

@Noratrieb
Copy link
Member Author

For benches, see my comments in #2214.

@RalfJung
Copy link
Member

RalfJung commented Jun 8, 2022

Thanks, this makes sense!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2022

📌 Commit 93db9a6 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jun 8, 2022

⌛ Testing commit 93db9a6 with merge 4d6eca1...

@bors
Copy link
Contributor

bors commented Jun 8, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 4d6eca1 to master...

@bors bors merged commit 4d6eca1 into rust-lang:master Jun 8, 2022
@Noratrieb Noratrieb deleted the faster-tag-partial-eq branch June 8, 2022 18:00
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.

4 participants