When encountering `&SmallImplCopy < &SmallImplCopy`, suggest copying #108372

estebank · 2023-02-22T23:48:56Z

When encountering a binary operation for two T: Copy where T is as small as a 64bit pointer, suggest dereferencing the expressions so the binary operation is inlined.

Mitigate the effects of #105259, particularly when match ergonomics is involved.

rustbot · 2023-02-22T23:49:01Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-02-22T23:49:04Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

estebank · 2023-02-22T23:49:39Z

@bors try

bors · 2023-02-22T23:49:47Z

⌛ Trying commit f2e8be54b78532fa835749035d7eda460b85d459 with merge f696d008ac44f498f11e01e4370309aab53c5dcd...

estebank · 2023-02-22T23:51:33Z

This lint might be more appropriate in clippy, but I wanted to see what the impact on crater would be.

bors · 2023-02-23T00:02:52Z

💔 Test failed - checks-actions

scottmcm · 2023-02-23T00:22:21Z

library/std/src/net/ip_addr.rs

@@ -1875,7 +1875,7 @@ impl PartialEq<IpAddr> for Ipv6Addr {
    fn eq(&self, other: &IpAddr) -> bool {
        match other {
            IpAddr::V4(_) => false,
-            IpAddr::V6(v6) => self == v6,
+            IpAddr::V6(v6) => *self == *v6,


Hmm, suggesting changes like this doesn't fill me with joy.

When in the compiler does the important difference between the two codegens happen? Does it need to be in THIR->MIR lowering, or could a mir-opt notice these cases early enough to give the same benefits?

Yeah, not suggesting this would be ideal. I wonder if the original problem is more with this happening in loops in particular. Having said that, I wanted to gauge how prevalent this is.

Looking at the number of places where we are warning in rustc itself, I fear that 1) actually landing the suggestion should be gated on detecting the expression being part of a loop and 2) there might be real perf gains on fixing the underlining codegen optimization issue or cleaning up the ecosystem with the aid of this lint.

compiler-errors · 2023-02-23T02:50:56Z

compiler/rustc_lint/src/builtin.rs

+            && size <= 8
+            && size_r <= 8


Should we hard-code 8 here? Or get the usize-size from the compiler?

We would then give different suggestions depending on what platform the developer happens to be in. That is a departure of what we've been doing so far.

compiler/rustc_lint/src/builtin.rs

bors · 2023-02-23T22:14:59Z

💔 Test failed - checks-actions

scottmcm · 2023-02-23T22:49:48Z

Mitigate the effects of #105259, particularly when match ergonomics is involved.

Unfortunately, I'm not sure it does.

If I take the example from the issue and manually deref all the &usizes,

 pub fn variant_X(input: &[(usize, usize, usize, usize)]) -> usize {
     input
         .iter()
-        .filter(|(a, b, c, d)| a <= c && d <= b || c <= a && b <= d)
+        .filter(|(a, b, c, d)| *a <= *c && *d <= *b || *c <= *a && *b <= *d)
         .count()
 }

Then I still get the same branchy code: https://rust.godbolt.org/z/8KEn563eY 🙁

estebank · 2023-02-24T01:04:36Z

@bors try @rust-timer queue

rust-timer · 2023-02-24T01:04:38Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2023-02-24T01:04:46Z

⌛ Trying commit f69f2e381683371996771d7c1c09536a98f3baa2 with merge 789cac255072eb4c1b81cf93275aee3003c63106...

bors · 2023-02-24T03:50:18Z

💔 Test failed - checks-actions

kadiwa4 · 2023-02-26T10:53:10Z

Why does it matter if the type is Copy? *a == *b won't copy (or move) a or b anywhere; they'll be immediately referenced again and then passed to PartialEq::eq.
Doesn't the machine always need to "chase pointers" if the type being compared is a reference?

scottmcm · 2023-02-27T01:33:38Z

@kadiwa4 It matters for primitives. Compare the MIR in https://rust.godbolt.org/z/MsWxfn6s8.

bors · 2023-02-27T22:54:56Z

☔ The latest upstream changes (presumably #108538) made this pull request unmergeable. Please resolve the merge conflicts.

… MIR Today, if you're comparing `&&T`s, it ends up auto-reffing in HIR. So the MIR ends up calling `PartialEq/Cmp` with `&&&T`, and the MIR inliner can only get that down to `&T`: <https://rust.godbolt.org/z/hje6jd4Yf>. So this adds an always-run pass to look at `Call`s in MIR with `from_hir_call: false` to just call the correct `Partial{Eq,Cmp}` implementation directly, even if it's debug and we're not running the inliner, to avoid needing to ever monomorphize a bunch of useless forwarding impls. This hopes to avoid ever needing something like rust-lang#108372 where we'd tell people to manually dereference the sides of their comparisons.

rustbot assigned petrochenkov Feb 22, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 22, 2023

estebank mentioned this pull request Feb 22, 2023

&usize::<= is much slower than usize::<= #105259

Open

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2023